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: Silent "Policy CMP0167 is not set" #259

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jul 4, 2024

This PR silents a developer warning when using >=CMake 3.30.

Closes #257.

@fanquake
Copy link

fanquake commented Jul 4, 2024

According to https://gitlab.kitware.com/cmake/cmake/-/issues/19402, Boost 1.70 and later is sufficient to use the new behaviour. Given our minimum required is 1.73, is there a reason we can't just use the new behaviour? Also:

Since polices are not feature toggles and should generally not be set to OLD, such a policy will make sense only when versions of Boost prior to 1.70 have fallen out of use.

@hebasto hebasto added this to the Ready for master milestone Jul 4, 2024
@hebasto hebasto added the enhancement New feature or request label Jul 4, 2024
@hebasto
Copy link
Owner Author

hebasto commented Jul 4, 2024

According to https://gitlab.kitware.com/cmake/cmake/-/issues/19402, Boost 1.70 and later is sufficient to use the new behaviour. Given our minimum required is 1.73, is there a reason we can't just use the new behaviour? Also:

Since polices are not feature toggles and should generally not be set to OLD, such a policy will make sense only when versions of Boost prior to 1.70 have fallen out of use.

The boost_1_81_0.tar.gz package, which we use in depends, does not contain a CMake configuration file.

As we use only Boost headers and finding them is quite a trivial task, we can introduce our own module, say FindBoostHeaders, as a follow up after migration to CMake.

@fanquake
Copy link

fanquake commented Jul 4, 2024

The boost_1_81_0.tar.gz package, which we use in depends, does not contain a CMake configuration file.

Not sure what you mean. It doesn't currently exist in depends because we just copy the headers, rather than compiling / installing. If you build/install you'll get a BoostConfig.cmake.

As we use only Boost headers and finding them is quite a trivial task, we can introduce our own module, say FindBoostHeaders,

So reimplement and maintain the old behaviour (I guess something like the Boost macro in master) that CMake is getting rid of? Wouldn't it be better to just use the CMake config file in depends, and the modern behaviour?

@hebasto
Copy link
Owner Author

hebasto commented Jul 4, 2024

The boost_1_81_0.tar.gz package, which we use in depends, does not contain a CMake configuration file.

Not sure what you mean. It doesn't currently exist in depends because we just copy the headers, rather than compiling / installing. If you build/install you'll get a BoostConfig.cmake.

Right. We have to modify the way how the boost package is provided in depends. That seems a bit invasive at this point.

As we use only Boost headers and finding them is quite a trivial task, we can introduce our own module, say FindBoostHeaders,

So reimplement and maintain the old behaviour (I guess something like the Boost macro in master) that CMake is getting rid of? Wouldn't it be better to just use the CMake config file in depends, and the modern behaviour?

I don't know what is better. Anyway, there is no need to reimplement the whole CMake's module, which includes handling libraries.

@hebasto
Copy link
Owner Author

hebasto commented Jul 12, 2024

@fanquake @theuni

While bitcoin#30434 is drafted, can we proceed with this approach?

@hebasto
Copy link
Owner Author

hebasto commented Jul 15, 2024

Rebased.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK, but please add a comment about this and Boost_NO_BOOST_CMAKE. Something like:

We cannot rely on find_package(Boost ... ) to work properly without Boost_NO_BOOST_CMAKE set until we require a more recent boost because upstream did not ship proper CMake files until 1.82.0.

Silent "Policy CMP0167 is not set" warning and document usage of the
`Boost_NO_BOOST_CMAKE` variable.
@hebasto
Copy link
Owner Author

hebasto commented Jul 16, 2024

ACK, but please add a comment about this and Boost_NO_BOOST_CMAKE. Something like:

We cannot rely on find_package(Boost ... ) to work properly without Boost_NO_BOOST_CMAKE set until we require a more recent boost because upstream did not ship proper CMake files until 1.82.0.

Thanks! The comment has been updated.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK b3448f1

@hebasto hebasto merged commit 1732a6b into cmake-staging Jul 16, 2024
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy CMP0167 is not set: The FindBoost module is removed.
3 participants