build: set cmake policy version to 3.31

[purpleKarrot]

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.

[ryanofsky]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[ryanofsky]

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]

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]

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.