Skip to content

Conversation

purpleKarrot
Copy link
Contributor

Please see cmake_minimum_required() for details.

This PR does not change the miminum required version of CMake; it just changes the policy version, which would otherwise be identical to the minimum required version (3.8).

A policy version of less than 3.10 causes a warning when using CMake 3.31. The policy version should always be set to the latest version that a project has been tested with.

Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 300278d. Thanks for the fix. I've seen this warning before but didn't understand what caused it. This explanation and fix make sense.

@hebasto
Copy link
Member

hebasto commented Feb 20, 2025

The policy version should always be set to the latest version that a project has been tested with.

Doing so will make the build system's behaviour dependent on the CMake version.

@ryanofsky
Copy link
Collaborator

Doing so will make the build system's behaviour dependent on the CMake version.

IIUC, that does seem like a possible downside to this change. Before 300278d with cmake_minimum_required(VERSION 3.8) specified everybody who is using this build will see cmake 3.8 behaviors no matter what version of cmake they are using. But after 300278d with cmake_minimum_required(VERSION 3.8...3.31) people who are using cmake 3.8 will get cmake 3.8 behaviors, people who are using cmake 3.12 will get 3.12 behaviors, 3.20 will see 3.20 behaviors, etc. Cmake will stop trying to be compatible with version 3.8 and only try to be compatible with version 3.31, so people using cmake versions after 3.31 will still see 3.31 behaviors.

On the other hand, it does seem better to use 3.31 behaviors instead of 3.8 behaviors as long as it doesn't cause bugs. Because if it does cause bugs we will need to fix them eventually anyway and it would be better to fix them sooner rather than later. Also in the worst case if any bugs are difficult to fix properly or difficult to fix while maintaining compatibility we can always set cmake_policy SET to restore specific old behaviors that we want.

So overall this change seems good, even if it might expose some problems in the short run.

@ryanofsky
Copy link
Collaborator

I guess another possible downside of this change is it increases the chances that we accidentally break compatibility with old versions of cmake without knowing about it.

For example right now maybe we can be reasonably assumed that we are not depending on cmake 3.20 features because our cmake policy version is 3.8 so those features aren't enabled for us. But if we increase our policy version to 3.31 then new features may become available and we may rely on them without knowing about it. I'm not sure if cmake can warn us when we are relying on a new feature without checking for it and need to increase our minimum version.

@ryanofsky
Copy link
Collaborator

Would welcome more input on this. I think I'm still inclined to just test this a little bit and merge it and fix any possible bugs that are exposed. But we could also consider raising minimum version or reducing maximum version to try to ensure more predictability in the future. Open to opinions or more information if I'm not understanding this correctly.

@purpleKarrot
Copy link
Contributor Author

Whenever a new version of CMake comes out, you should test your project with that new version without changing the policy version.

For all new policies, cmake will execute two code paths: the one with the old behavior and the one with the new behavior. If the results mismatch, cmake will print a warning that your project is affected by the policy and then will use the old result. You can silence the warning by setting the policy to OLD, but this should be temporarily only, because the old behavior is deprecated by definition.

Once you fixed a policy warning by avoiding ambiguous cmake code, you should set the policy to NEW. Once you fixed all policy warnings, you may increase the policy version. You can then be sure that behaviour does not dependent on the CMake version.

Note that cmake_minimum_required(VERSION 3.8) does not prevent using features that are introduced later. The risk of accidental breakage is real, but it is not increased with this change. The only way to actually guarantee compatibility with version 3.8 is to use that version in CI.

@hebasto
Copy link
Member

hebasto commented Feb 21, 2025

As a part of the Bitcoin Core project, libmultiprocess should ensure a stable build environment.

Testing all available policies introduces unneeded burden for developers.

Also see hebasto/bitcoin#143.

@purpleKarrot
Copy link
Contributor Author

Testing all available policies introduces unneeded burden for developers.

You do that multiple times a day. Every time you run cmake, all available policies are tested. If no policy warnings are printed, it means that you are not affected by the breaking changes those policies are introduced for. You should trust cmake and increase the policy version in this case.

You don't turn cmake into a hermetic buildsystem by keeping policies unset. You should use them the way it is intended by their developers.

@maflcko
Copy link
Contributor

maflcko commented Feb 21, 2025

I'd expect the issue be mostly with newly written build code, because it puts the burden on developers to be aware of any policy changes in the version range? Usually developers are on the higher end of the version range, so are testing the new policies (if any), though users may be on older cmakes and then run into bugs. Sure, CI should be checking the minimum version, but it can't cover all possible user systems. Thus, setting only a single version (the minimum) is a trade-off to reduce the risks here, but I may be misunderstanding, or missing something.

@maflcko
Copy link
Contributor

maflcko commented Feb 22, 2025

So if my understanding in the previous comment is correct, I presume a possible alternative fixes would be to simply change 3.8 into 3.10 to avoid the cmake warning. As part of reviewing of that fix, one would check all policy changes between 3.8 and 3.10 and confirm that they do not affect the build, or are already accounted for. Given that a build with cmake 3.10 (or any version up to 3.30) did not issue any warnings, there are likely no policy changes affecting the project and the change should be fine. Also, there should be no need to support 3.8 at all, because the project here requires a C++20 compiler. Any system that comes with a C++20 compiler should also support running a more recent cmake than 3.8.

Going further, if someone tried to actually use cmake 3.8 (up to cmake 3.11), it should already fail because the value 20 isn't defined.

So I submitted this fix in #164

@ryanofsky
Copy link
Collaborator

For now I merged #164 which raises minimum version to 3.12, since that seems like the most obvious fix for the warning.

I am still interested in setting in specifying a max version to use a higher default policy version though. It seems to me like there are different risks to whichever policy version you chose (though the risks are minor and theoretical in all cases). If you choose an old policy version, you are opting into old deprecated behaviors and relying on cmake's ability to emulate old versions of itself. If you set a new policy version, you put developers using older cmake versions at a disadvantage because you aren't testing the old policies they might be using.

IMO the best thing to do would be to disable application of old cmake policies and use non-deprecrated policies in our release build, which uses cmake 3.24.2 (it is also 3.24.2 according to guix time-machine --commit=53396a22afc04536ddf75d8f82ad2eafa5082725 -- shell cmake-minimal -- cmake --version). So IMO it would be good to set 3.24 as max version, so developers are free to use older or new versions of cmake and their builds will always match the release build as closely as possible.

@purpleKarrot
Copy link
Contributor Author

purpleKarrot commented Feb 24, 2025

According to the cmake documentation, the <min>...<max> syntax means that the project is expected to work with both the OLD and NEW behaviors of policies introduced between those versions.

For earlier versions than <min>, only the NEW behavior is supported and for later versions than <max> only the OLD behavior is supported.

Once a new version of cmake introduces a new deprecation, it is recommended to update affected code as soon as possible. It does not matter what version is used to build releases. Once we know that a certain feature of cmake is going to be removed in the future, we should stop using that particular feature, not continue using it until official releases are built with a version that no longer has it.

@purpleKarrot
Copy link
Contributor Author

Sure, CI should be checking the minimum version, but it can't cover all possible user systems.

It does not have to cover all possible user systems; it has to cover all possible policies. This is archived by testing both <min> (where all policies in the <min>...<max> range are OLD) and <max> (where all policies in that range are NEW). With only two CI builds, you are covering both OLD and NEW for all possible policies. That is no burden.

@maflcko
Copy link
Contributor

maflcko commented Feb 25, 2025

Sure, CI should be checking the minimum version, but it can't cover all possible user systems.

It does not have to cover all possible user systems; it has to cover all possible policies. This is archived by testing both <min> (where all policies in the <min>...<max> range are OLD) and <max> (where all policies in that range are NEW). With only two CI builds, you are covering both OLD and NEW for all possible policies. That is no burden.

According to the cmake documentation, there are policies that only take effect on some systems, so I don't think testing on a single system the min and max cmake version is sufficient.

For example, https://cmake.org/cmake/help/latest/policy/CMP0141.html says: "The policy setting takes effect [...] whose compiler targets the MSVC ABI."

IMO the best thing to do would be to disable application of old cmake policies

Isn't this what is happening in current master already? (Pin the cmake version and address any policy warnings by dropping support for (deprecated or) old cmake policies)

release build

If this project is tied to Bitcoin Core, it could make sense to just take over the min version from there: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/CMakeLists.txt#L10

Since this project has no CI and I presume no one is testing on ancient versions of Cmake anyway, so the only testing is on recent versions of cmake, or the versions of cmake used in the CI of bitcoin core.

@maflcko
Copy link
Contributor

maflcko commented Mar 12, 2025

I guess this can be closed? If not, it would be good to address the outstanding feedback, and to adjust the pull description to reflect the latest state.

@ryanofsky
Copy link
Collaborator

re: #163 (comment)

According to the cmake documentation, there are policies that only take effect on some systems, so I don't think testing on a single system the min and max cmake version is sufficient.

This is true, but I don't see it as a goal to have test coverage for esoteric options on all systems. I think best strategy is just to prefer modern defaults over deprecated defaults, and explicitly document whatever defaults we tested and have most confidence in.

IMO the best thing to do would be to disable application of old cmake policies

Isn't this what is happening in current master already? (Pin the cmake version and address any policy warnings by dropping support for (deprecated or) old cmake policies)

No, current master is using policies from 3.12 which enables policies up to CMP0075. The latest cmake release is cmake 3.31 which enables policies up to CMP0180. So we are opting into 180-75=105 outdated policies which could make builds slower and less reliable and worse for users, and which are deprecated and less well tested upstream.

If this project is tied to Bitcoin Core, it could make sense to just take over the min version from there: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/CMakeLists.txt#L10

This doesn't make a lot of sense to me. IMO, purpose of the min version should just be to document the minimum version that should work, and not to preemptively break compatibility with older versions of cmake when there is no reason to believe they have any problems.

i do think it could be reasonable to set max version here to the policy version used by bitcoin core to avoid opting into newer cmake policies than are used by bitcoin core releases. This could be 3.22 since that's the policy version used in the cmake file you linked to, or 3.24 since that's the version of cmake actually used to build releases mentioned earlier. But it also seems reasonable to opt into new all new policies like this PR is doing, set the max version to 3.31 and just document the fact that building with 3.31 is known to work.

I don't think any of this is critically important, and I doubt it actually matters what max version we set here in practice. But it seems pretty wonky to me to intentionally disable improvements that have been made to cmake since 3.12, so I think this PR is better than the status quo.

If there are objections to this PR that 3.31 is too new, those could be reasonable, and maybe we should go with 3.22 or 3.24 as the max version instead of 3.31. But IMO it would not be good to set 3.22 or 3.24 min version if there's no real reason to require those versions, and I don't think the 3.12 policy set is the one we should be opting into, so I agree with purpleKarrot that we should be setting a max version to opt into newer policies.

Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK f507c50. Seems like an improvement to me to opt into new, non-deprecated policies by default if we've done a little testing with them and believe the build should work. A theoretical downside of this change is that that it could put users of older cmake versions at a disadvantage, because we will no longer be emulating old versions of cmake in our own builds and might unintentionally break things for them by relying on newer policies. But I don't believe this will happen in practice and if it does happen I think it would be useful to have bug reports so we can know if we are relying on particular policies.

Would also be happy with a different change that sets max version to something less than 3.31 to be more conservative and not use the latest policies, but I don't think it makes sense to use 3.12 policies.

@hebasto
Copy link
Member

hebasto commented Mar 12, 2025

If this PR is merged, what actions will be taken when a new CMake release comes out?

What are criteria to bump the <policy_max> option?

@ryanofsky
Copy link
Collaborator

If this PR is merged, what actions will be taken when a new CMake release comes out?

My plan would be to wait for someone to open another PR, wait for it to get acks, and merge it. If someone is interested in using newer cmake policies, they should have motivation to submit a PR. If no one is interested, it should also be ok to stick with oldier policies we are using. But we should not intentionally prefer old and deprecated policies.

@ryanofsky
Copy link
Collaborator

I think I will probably merge this PR soon. It seems like a safe, practical change to opt the build into more widely used, modern recommended cmake default behaviors instead of enabling dozens of backwards compatibility policies that cause modern cmake versions try to emulate a cmake release that is 7 years old.

I do think statement in the PR description "The policy version should always be set to the latest version that a project has been tested with" is probably a little too strong, because this change comes with some downside of making it possible for the build to unintentionally rely on new cmake policies without us realizing it (which is also possible without this change, but less likely). In the end it's probably best to set the max version to a recent version that we think some number of users and developers are using, without needing it to be the very latest version.

@fanquake
Copy link
Member

Note that the latest available CMake version is now also 4.0.2.

@maflcko
Copy link
Contributor

maflcko commented May 15, 2025

If this project is tied to Bitcoin Core, it could make sense to just take over the min version from there: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/CMakeLists.txt#L10

This doesn't make a lot of sense to me. IMO, purpose of the min version should just be to document the minimum version that should work, and not to preemptively break compatibility with older versions of cmake when there is no reason to believe they have any problems.

i do think it could be reasonable to set max version here to the policy version used by bitcoin core to avoid opting into newer cmake policies than are used by bitcoin core releases. This could be 3.22 since that's the policy version used

Why does it not make sense?

This repo has no CI, and no developers that are testing with such an old cmake version. (As evident from #164). Also, it is unclear what the goal would be to even try to support such ancient versions. libmultiprocess is currently only used by Bitcoin Core, which requires 3.22, so any developer or users will use that version (or more likely a later one) anyway.

Maybe it is fine to derive from that in the future, when there is an actual need or use-case, but absent that it just seems simple and consistent to follow Bitcoin Core.

@ryanofsky
Copy link
Collaborator

Why does it not make sense?

The cmake_minimum_required call does two different things:

1 - It enables & disables various cmake compatibility policies
2 - It enables build errors when you are using a version of cmake that is too old to work.

The focus of this PR is (1) and this PR only changes (1) not (2).

But with regard to (2), I am saying in that comment that it does not make sense to me to change minimum required version of cmake from 3.12 to something else when the cmake build here is minimal and only uses rudimentary cmake features.

I'm not saying we should try to support old versions of cmake or go out of our way to test old versions of cmake. (Though if someone wants to open a PR testing an old version of cmake in CI, it'd be more than welcome). All I'm saying is that we should not trigger a fatal error preemptively without a factual reason to believe there is some problem.

Maybe it is fine to derive from that in the future, when there is an actual need or use-case, but absent that it just seems simple and consistent to follow Bitcoin Core.

This would seem to create a dependency doesn't need to exist. Like in the future if bitcoin core starts relying on a new cmake feature an needs to raise its minimum version, I don't see what the point would be of opening a separate PR to raise the minimum version here. It seems better to just document what we think the actual minimum version here is and throw errors if an older version is used. It does not seem good to tie the repositories together with unnecessary requirements.

That said, if you are concerned that versions of cmake before 3.22 are bad or broken or scary in some way and want to trigger a fatal error on use, I wouldn't object to a PR that raised the minimum version here. I just think it would be misleading and not practically helpful since bitcoin core already requires 3.22. It should also be unrelated to this PR since this PR is leaving the minimum version alone and only raising the policy version.

@hebasto
Copy link
Member

hebasto commented May 15, 2025

Here are a couple of examples of policies that change the build system's behaviour depending on the CMake version in use:

That is the main reason I'm reluctant on use the ...<policy_max> option.

@ryanofsky
Copy link
Collaborator

That is the main reason I'm reluctant on use the ...<policy_max> option.

CMP0117 stops specifying a redundant rtti flag
CMP0141 stops specifying harmful debug information flags

You didn't say what your reasoning is, but it seems like from these examples you don't trust new cmake releases to set good default policies and want to opt into old policies cmake developers do not recommend... unless those policies are so old that they are below some minimum version of cmake we can't support or are choosing not to support.

This seems like precarious situation to opt into, where you want to allow running new versions of cmake with old policies from some arbitrary version instead the actual default policies they ship with and recommend.

I do understand your previously stated concern that enabling recent policies will make the "build system's behaviour dependent on the CMake version". But that concern exists regardless, not just with cmake but with all the tools we are using. It's a legitimate thing to be worried that the compilation behavior may be dependent on on the version of the compiler that is used. Maybe you are concerned GCC 14 will have some problems that GCC 13 doesn't have. But the ideal solution to that is to use GCC 13, not use GCC 14 with a bunch of flags to emulate old behaviors. That's a reason our official builds use a fixed set of dependencies, including a fixed cmake dependency. But letting version of cmake runtime and library vary up the the bleeding edge while statically setting a much older policy version seems like a dubious practice, IMO.

@DrahtBot
Copy link

DrahtBot commented Jun 23, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #209 (cmake: Increase cmake policy version by ryanofsky)
  • #175 (Set cmake_minimum_required(VERSION 3.22) by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@@ -2,7 +2,7 @@
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

cmake_minimum_required(VERSION 3.12)
cmake_minimum_required(VERSION 3.12...3.31)
Copy link
Member

Choose a reason for hiding this comment

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

Should we increase this to 4.1.1 now, if we are planning to make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4.1, since no policies are introduced in patch releases.

@ryanofsky
Copy link
Collaborator

ryanofsky commented Sep 8, 2025

I think I'd like to merge this PR soon if there's not a strong objection. Josibake pointed out that cmake documentation explicitly says "Projects should be updated to use the NEW behaviors of the policies as soon as possible," and I think it just makes sense to not opt into deprecated policies without a specific reason for doing that.

I don't think it matters too much which exact policy version is used, but it would make sense to specify policies that have some testing in CI. Currently the nix CI jobs are using cmake-3.31.6, the macos CI job is probably using 4.1.1, and it's unclear what versions of cmake the openbsd and netbsd jobs are using. (Should probably update the cmake script to print the cmake version.) (EDIT: This is done in #207) Bitcoin core is still using policy version 3.22 with different runtime cmake versions in CI, and using pinned cmake runtime version 3.24 to build binary releases.

This PR changes the cmake policy version from 3.12 to 3.31 without changing the minimum cmake version, which is still 3.12. I think it's good this PR is leaving the minimum version alone, because the policy version and minimum version are different concepts, and effects of changing them are not the same. There's a separate PR #175 that does change the minimum version, and I also started working on a change to add CI coverage to the olddeps job for the current minimum version. (EDIT: This is done in #208)

An alternative to this PR could choose to use a policy version of 3.22 instead of 3.31 like bitcoin core does. This would go against what cmake documentation recommends and opt into deprecated behavior in builds, but it could reduce variance between cmake versions and might catch bugs that would be unnoticed if users are using older versions of cmake and developers are using newer versions of cmake, as discussed above.

Another alternative or possible followup, if we are worried about not using a consistent set of policies in the bitcoin core build, might be to wrap the cmake_minimum_required call in if (NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION) to let bitcoin core policies take precedence.

@Sjors
Copy link
Member

Sjors commented Sep 8, 2025

I'm generally fine with focussing support on modern environments and only adding support for older stuff if there's a need for it. Since Bitcoin Core requires 3.22 it would make little sense to support anything older.

If CI passes with 4.1 that's fine too.

@hebasto
Copy link
Member

hebasto commented Sep 8, 2025

If CI passes with 4.1 that's fine too.

Does CI have any job that uses CMake 4.1?

@ryanofsky
Copy link
Collaborator

Does CI have any job that uses CMake 4.1?

Looks like not currently. I though mac os ci job might be using it because 4.1.1 is currently stable in homebrew, but in #207 the macos ci job https://github.com/bitcoin-core/libmultiprocess/actions/runs/17552747815/job/49848966387?pr=207 outputs 3.31.6

ryanofsky added a commit that referenced this pull request Sep 8, 2025
d603dcc ci: output CMake version in CI script (fanquake)

Pull request description:

  Mentioned here: #163 (comment).

ACKs for top commit:
  hebasto:
    ACK d603dcc.
  ryanofsky:
    Code review ACK d603dcc

Tree-SHA512: 50023ce6f3f301e9138e77e3f0f0a61546b08d873e8b56a91fca17ae743524bf26ba5bcc523f80cb7408317b112ba2a4a8bd048936fbfbdb6825c2954366cd98
@hebasto
Copy link
Member

hebasto commented Sep 8, 2025

Josibake pointed out that cmake documentation explicitly says "Projects should be updated to use the NEW behaviors of the policies as soon as possible," and I think it just makes sense to not opt into deprecated policies without a specific reason for doing that.

At some point, I stopped treating the CMake docs as a reference for best practices.

I still insist that Bitcoin Core's build system should have fixed effective policies, independent of the CMake version in use.

Another point is that none of the reviewers of this PR mentioned whether they actually analysed the effects of the new policies up to CMP0180.

As a subproject of Bitcoin Core, the most straightforward way to enforce a fixed policy is not to use a version range. This approach is already used in other subprojects, such as libsecp256k1 and minisketch.

An alternative to this PR could choose to use a policy version of 3.22 instead of 3.31 like bitcoin core does.

Why not simply close this one in favour of #175?

Another alternative or possible followup, if we are worried about not using a consistent set of policies in the bitcoin core build, might be to wrap the cmake_minimum_required call in if (NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION) to let bitcoin core policies take precedence.

This would be my second choice, if people agree that the extra complexity is worth it.

@ryanofsky
Copy link
Collaborator

@hebasto can you clarify what your concern is exactly? If you want old cmake behaviors, you should use old versions of cmake. If you want new cmake behaviors you should use new versions of cmake. It makes no sense to use new versions of cmake with policy settings causing them to emulate bugs and quirks of older versions released 7 or 4 years ago. This is not what cmake policies were designed for or what cmake documentation recommends for good reason. Cmake policies are a tool for controlling rollout of new changes. They are not a guarantee of stability and if anything they will make builds less stable as policy and runtime versions diverge and receive less testing.

I still insist that Bitcoin Core's build system should have fixed effective policies, independent of the CMake version in use.

Bitcoin core's build system pins its cmake version via guix similar to how it pins other dependencies. There is nothing special about cmake in this respect. I can understand why it's important to pin the cmake version, but it's doesn't make sense to insist that cmake behavior be the same "independent of the cmake version." The point of different versions is to have different behavior.

Another point is that none of the reviewers of this PR mentioned whether they actually analysed the effects of the new policies up to CMP0180.

Not sure what you are looking for here, but happy to review or test anything you'd like me to.

Why not simply close this one in favour of #175?

#175 is setting the minimum version incorrectly, but if it can be updated to just change the policy version without changing the minimum version it seems fine.

This would be my second choice, if people agree that the extra complexity is worth it.

If @purpleKarrot can add if (NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION) and it would unblock this PR, I'd be happy to see that. Would also be happy to see this added in a separate PR or to open a PR which adds this. I do think it would be an improvement to use policies that are 4 years old instead of 7 years old in the bitcoin build at least.

@hebasto
Copy link
Member

hebasto commented Sep 8, 2025

@hebasto can you clarify what your concern is exactly? If you want old cmake behaviors, you should use old versions of cmake. If you want new cmake behaviors you should use new versions of cmake.

None of these are my intentions. As I wrote earlier, Bitcoin Core's build system behavior should not be affected by the CMake version in use.

It makes no sense to use new versions of cmake with policy settings causing them to emulate bugs and quirks of older versions released 7 or 4 years ago. This is not what cmake policies were designed for or what cmake documentation recommends for good reason. Cmake policies are a tool for controlling rollout of new changes. They are not a guarantee of stability and if anything they will make builds less stable as policy and runtime versions diverge and receive less testing.

The opposite. This is exactly what policies were designed for. From Professional CMake: A Practical Guide 21st Edition, Section 13.1:

This gives projects confidence that developers should be able to update to a newer version of CMake at their convenience, and the project will still build as it did before.

Again, I would strongly advise against treating the official docs as best practices. CMake has a history of introducing questionable features aimed at paid customers.

I still insist that Bitcoin Core's build system should have fixed effective policies, independent of the CMake version in use.

Bitcoin core's build system pins its cmake version via guix similar to how it pins other dependencies. There is nothing special about cmake in this respect. I can understand why it's important to pin the cmake version, but it's doesn't make sense to insist that cmake behavior be the same "independent of the cmake version." The point of different versions is to have different behavior.

Following this logic, Bitcoin Core should have CI jobs that running the CMake version pinned in Guix, and some developers would be expected to do the same, which seems rather awkward.

Why not simply close this one in favour of #175?

#175 is setting the minimum version incorrectly...

My apologies, but I must disagree.

@ryanofsky
Copy link
Collaborator

None of these are my intentions. As I wrote earlier, Bitcoin Core's build system behavior should not be affected by the CMake version in use.

Sorry, I don't understand. This seems analogous to saying the compiler behavior should not be affected by the gcc version in use. The reason different cmake versions exist is to have different behavior. Newer versions should have newer features, fewer bugs, faster builds, etc. Again it would help if you can explain how cmake is different from other dependencies like gcc or sqlite where different versions have different behaviors and we don't insist on there being unnecessary flags to make new versions act like old versions.

The opposite. This is exactly what policies were designed for. From Professional CMake: A Practical Guide 21st Edition, Section 13.1:

This gives projects confidence that developers should be able to update to a newer version of CMake at their convenience, and the project will still build as it did before.

This quote is saying the same thing I said: "Cmake policies are a tool for controlling rollout of new changes." The quote says "a newer version at their convenience" not "new versions 7 years from now." Cmake policies are a tool for smoother upgrades, not form of pinning.

Following this logic, Bitcoin Core should have CI jobs that running the CMake version pinned in Guix, and some developers would be expected to do the same, which seems rather awkward.

Yes, it would make sense for there to be a CI job that tests the same cmake version as guix. I don't think this job would be particularly likely to break and detect issues, but it'd be natural to have and doesn't seem like it would be more awkward to set up than any other CI job. Of course many developers do guix builds as well. I think I don't understand what is bad or awkward about either of these things.

#175 is setting the minimum version incorrectly...

My apologies, but I must disagree.

To be specific, #175 would result in error messages which are not accurate like "CMake 3.22 or higher is required. You are running version [...]." These errors would be incorrect because this project doesn't rely on any new cmake features and versions back to 3.12 work well, even with some CI coverage in #208.

I'd be happy with a PR that set cmake_minimum_required(VERSION 3.12...3.22) accurately representing the required minimum version as 3.12 and increasing the policy version to 3.22. I would even favor that change if I were concerned that "CMake has a history of introducing questionable features aimed at paid customers." Since that is not a concern I actually have, I am also happy with any other change that increases the policy version and without changing and misrepresenting the minimum required version.

Maybe as a possible way forward you could clarify which of the following variants you might dislike the least?

cmake_minimum_required(VERSION 3.12...3.31)
cmake_minimum_required(VERSION 3.12...3.22)
if(NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION)
  cmake_minimum_required(VERSION 3.12...3.31)
endif()
if(NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION)
  cmake_minimum_required(VERSION 3.12...3.22)
endif()

@hebasto
Copy link
Member

hebasto commented Sep 8, 2025

Maybe as a possible way forward you could clarify which of the following variants you might dislike the least?

cmake_minimum_required(VERSION 3.12...3.31)
cmake_minimum_required(VERSION 3.12...3.22)
if(NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION)
  cmake_minimum_required(VERSION 3.12...3.31)
endif()
if(NOT DEFINED CMAKE_MINIMUM_REQUIRED_VERSION)
  cmake_minimum_required(VERSION 3.12...3.22)
endif()
if(DEFINED CMAKE_MINIMUM_REQUIRED_VERSION)
  cmake_minimum_required(${CMAKE_MINIMUM_REQUIRED_VERSION})
else()
  cmake_minimum_required(VERSION 3.12...4.1)
endif()

@purpleKarrot
Copy link
Contributor Author

A new version of CMake is released. Project author tries it out and gets a warning that the project is affected by a new policy. Project author fixes the project so that it is no longer affected by the policy (eg by fixing an invalid regex, by properly quoting a string, by replacing a deprecated command invocation, by searching for a packages in config mode only, or similar). After making the change, CI verifies that the change did not raise the minimum requirement: Using an old versions of CMake still works. Project author increases the policy version to prevent regressions (like the re-introduction of invalid regexes, wrongly quoted strings, deprecated commands, deprecated find-modules, etc).

if(...)
  cmake_minimum_required(...)
endif()

Setting the policy version is a statement of the project author that the project is not affected by policy changes. Why should this be set from outside?

@Sjors
Copy link
Member

Sjors commented Sep 9, 2025

@purpleKarrot what you describe makes sense to me, more so than the documentation, maybe we should paste that as a comment :-)

@ryanofsky
Copy link
Collaborator

Setting the policy version is a statement of the project author that the project is not affected by policy changes. Why should this be set from outside?

@purpleKarrot it makes sense to me to use the policy version the way you describe, as an upgrade mechanism and way of documenting compatibility with new cmake versions as they are released.

However, the bitcoin core project is not using it this way, because the bitcoin core policy version is hardcoded to 3.22 even though most developers are using newer versions of cmake. Hebasto and marco also have expressed a preference for #175 instead of this PR, setting the libmultiprocess policy version to 3.22 to match bitcoin's policies.

As far as I can tell, hebasto and maybe marco are not looking at the cmake policy version setting as an upgrade mechanism and way to provide a smooth transition to new cmake versions as you describe. Instead they see the cmake policy version as a pinning mechanism: a way to guarantee old cmake behaviors are used regardless of the user's environment and version of cmake they have installed. This way for example, if developers are using new versions of cmake, and users are using old versions of cmake, everybody will be be using old cmake policies, and if there are any problems with those policies, developers will know about them and fix them.

If hebasto wants the bitcoin core policy to be set to 3.22 and for libmultiprocess policy to match this when it is compiled as part of bitcoin core, I don't have a problem with supporting that even though I feel the approach has more downsides than upsides. (Downsides being disabling cmake features and bugfixes, not respecting user preferences, using cmake in less common configuration against cmake developer recommendations. Upside being a form of pinning where there is less variance between builds in different environments.)

@hebasto
Copy link
Member

hebasto commented Sep 9, 2025

Setting the policy version is a statement of the project author that the project is not affected by policy changes. Why should this be set from outside?

It’s not about setting the policy version from outside. The downstream project is simply asking the project author not to break its currently pinned policy behaviour. The project author chooses a pinned policy version for such a use case at their own discretion.

If hebasto wants the bitcoin core policy to be set to 3.22 and for libmultiprocess policy to match this when it is compiled as part of bitcoin core...

The code snippet I suggested does indeed align the policies for Bitcoin Core and libmultiprocess, but that’s just a (reasonable) simplification. All I’m asking is that a pinned policy, chosen in some way, be used when building as a subproject of Bitcoin Core.

@ryanofsky
Copy link
Collaborator

Thanks for clarification hebasto. That all makes sense to me and explains what you are looking for, except I am confused by the last sentence:

All I’m asking is that a pinned policy, chosen in some way, be used when building as a subproject of Bitcoin Core.

This last sentence seems to imply that you don't care how the pinned policy is chosen. But you do have a clear preference for pinning the policy to an old version of cmake, not a current version of cmake like this PR.

@hebasto
Copy link
Member

hebasto commented Sep 9, 2025

All I’m asking is that a pinned policy, chosen in some way, be used when building as a subproject of Bitcoin Core.

This last sentence seems to imply that you don't care how the pinned policy is chosen.

That's correct. However, it should be a value within the [3.12..3.22] range.

But you do have a clear preference for pinning the policy to an old version of cmake, not a current version of cmake like this PR.

How would you suggest implementing policy version pinning in the latter case?

@ryanofsky
Copy link
Collaborator

Thanks I think I see the discrepancy. I am using the word "pinned" to mean "fixed." This PR is setting a fixed policy version, so I would say this PR is pinning the policy version.

But I think you are using "pinned" to not simply mean "fixed" but "fixed to the minimum version of cmake that does not trigger a build error." You want it to be an error to build with any version of cmake below the specified policy version.

@ryanofsky
Copy link
Collaborator

I'm starting to work on a PR to implement your suggestion #163 (comment). The PR will add a new "newdeps" CI job test cmake 4.1 since I don't think any current CI jobs are testing it. (It will also test a newer version of cap'n proto to mirror the "olddeps" CI job)

@purpleKarrot
Copy link
Contributor Author

All I’m asking is that a pinned policy, chosen in some way, be used when building as a subproject of Bitcoin Core.

But why? If project A was written to support the NEW behavior for policy X, why would project B build project A as subproject with policy X unset? The only usecase I can imagine where a superproject may want to set the policy version of a subproject is this:

Project A was not updated in many years, and therefore still sets a very old policy version, like 2.8.12. It is however very stable; there are no issues building it with the latest policy version of CMake and the latest compilers. The only issue is that CMake version 4.1 treats it as an error to set a policy version below 3.10. Project B wants to build project A as subproject without modifying A's source code. It can do that by setting CMAKE_POLICY_VERSION_MINIMUM right before the add_subdirectory() call.

@hebasto, I think you are asking for a feature that CMake does not provide. There is no way to tell CMake to behave as version 3.22. The policy mechanism may look like such a feature, but it is something different. CMake actually does provide feature flags for features that are currently under development. But the policies are not feature flags.

@hebasto
Copy link
Member

hebasto commented Sep 9, 2025

@hebasto, I think you are asking for a feature that CMake does not provide. There is no way to tell CMake to behave as version 3.22. The policy mechanism may look like such a feature, but it is something different. CMake actually does provide feature flags for features that are currently under development. But the policies are not feature flags.

Hmm, I'm confused. The quote I mentioned earlier states that "the project will still build as it did before".

I'd like to avoid, for example, treating a compiler flag differently depending on the policy version.

@purpleKarrot

Mind elaborating your point please? Or demonstrate it?

@ryanofsky
Copy link
Collaborator

ryanofsky commented Sep 9, 2025

6e29c02 (#209) is my attempt to implement hebasto's suggestion in #163 (comment). It uses the purpleKarrot and the cmake documentation's recommended approach by default, but stops overriding the policy version set by parent projects in case they are using different practices. So in the bitcoin core build it update the libmultiprocess policy version from 3.12 to 3.22 like hebasto and marco seem to favor

@purpleKarrot
Copy link
Contributor Author

The quote I mentioned #163 (comment) states that "the project will still build as it did before".

Yes, in the sense that it will not suddenly start failing or introduce other incompatible changes. It however makes no promise to get exactly the same behavior. If you need exactly the same behavior for reproducibility, you need to pin down the executable.

To demonstrate, I have installed CMake version 3.22.6 and configured bitcoin core. I have also configured it with CMake version 4.0.3 and compared the results. You can see that there are a number of changes both in the CMake cache and in the generated build system:

CMakeCache.txt              |  121 +-                          
CMakeFiles/rules.ninja      | 1438 ++++++++--                  
build.ninja                 | 3867 ++++++++++++++++++--------- 

I then changed the policy version to 4.0 and configured again. The following changes are not surprising; we expect different behavior when we set a different policy. But the output above should convince you that pinning a policy will not guarantee exact behavior across different versions of CMake.

build.ninja                 | 6466 ++++++++++++++++++++++++++++++++++++++---- 

@hebasto
Copy link
Member

hebasto commented Sep 10, 2025

The quote I mentioned #163 (comment) states that "the project will still build as it did before".

Yes, in the sense that it will not suddenly start failing or introduce other incompatible changes. It however makes no promise to get exactly the same behavior. If you need exactly the same behavior for reproducibility, you need to pin down the executable.

To demonstrate, I have installed CMake version 3.22.6 and configured bitcoin core. I have also configured it with CMake version 4.0.3 and compared the results. You can see that there are a number of changes both in the CMake cache and in the generated build system:

CMakeCache.txt              |  121 +-                          
CMakeFiles/rules.ninja      | 1438 ++++++++--                  
build.ninja                 | 3867 ++++++++++++++++++--------- 

I then changed the policy version to 4.0 and configured again. The following changes are not surprising; we expect different behavior when we set a different policy. But the output above should convince you that pinning a policy will not guarantee exact behavior across different versions of CMake.

build.ninja                 | 6466 ++++++++++++++++++++++++++++++++++++++---- 

Thanks for this demonstration.

So, a CMakeLists.txt with cmake_minimum_required(VERSION 3.22) generates different underlying build system code depending on the CMake binary version being used.

What about the resulting compiler, linker or archiver invocation strings?

@ryanofsky
Copy link
Collaborator

ryanofsky commented Sep 10, 2025

It's good to continue discussion here, but I think ultimately this PR could be closed in favor of #209, since #209 includes fixes for things that break with newer versions and adds more CI coverage. It's also more conservative than than this PR in only updating policies from 3.12 to 3.22 in the bitcoin core build instead of from 3.12 to 3.31. It also gives parent projects more control over the policy version generally.

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.

7 participants