Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Improve Boost checks #224

Merged
merged 2 commits into from
Jun 10, 2024
Merged

cmake: Improve Boost checks #224

merged 2 commits into from
Jun 10, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jun 6, 2024

The first commit was requested offline.

The second commit speeds up configuration.

@hebasto
Copy link
Owner Author

hebasto commented Jun 6, 2024

The first commit was requested offline.

It addresses @fanquake's concerns:

Why does AddBoostIfNeeded check for two different boost test headers ? We don't do this in master.

Ok. So it's another Windows package manager specific work around, being applied to all platforms. It'd be good to document that.

#include <boost/test/unit_test.hpp>
" HAVE_BOOST_UNIT_TEST_H
)
check_include_file_cxx(boost/test/unit_test.hpp HAVE_BOOST_UNIT_TEST_H)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might be missing something, but this is still checking two headers.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Reworked.

Only one Boost.Test header is being checked now.

@hebasto hebasto force-pushed the 240606-cmake-FW branch 2 times, most recently from fa5c6c0 to daca415 Compare June 10, 2024 08:26
Copy link

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just think that comment could be slightly clearer

#include <boost/test/included/unit_test.hpp>
" HAVE_BOOST_INCLUDED_UNIT_TEST_H
)
# Some package managers, such as vcpkg, install Boost headers individually

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Some package managers, such as vcpkg, install Boost headers individually
# Some package managers, such as vcpkg, vendor Boost Test separately
# from the rest of the headers, so we have to check for it individually.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

hebasto added 2 commits June 10, 2024 14:54
Add vcpkg-specific comment.
Switch to `check_include_file_cxx` when checking headers.
This speeds up configuration.

Only one Boost.Test header is being checked now.
@hebasto hebasto merged commit 8ce80b9 into cmake-staging Jun 10, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants