build: set cmake policy version to 3.31 #163
Conversation
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 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 So overall this change seems good, even if it might expose some problems in the short run. |
|
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. |
|
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. |
|
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 Once you fixed a policy warning by avoiding ambiguous cmake code, you should set the policy to Note that |
|
As a part of the Bitcoin Core project, Testing all available policies introduces unneeded burden for developers. Also see hebasto/bitcoin#143. |
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. |
|
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. |
|
So if my understanding in the previous comment is correct, I presume a possible alternative fixes would be to simply change Going further, if someone tried to actually use cmake 3.8 (up to cmake 3.11), it should already fail because the value So I submitted this fix in #164 |
|
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 |
|
According to the cmake documentation, the For earlier versions than 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
force-pushed
the
cmake-policy
branch
from
300278d to
f507c50
Compare
It does not have to cover all possible user systems; it has to cover all possible policies. This is archived by testing both |
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."
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)
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. |
|
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. |
|
re: #163 (comment)
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.
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.
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. |
There was a problem hiding this comment.
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.
|
If this PR is merged, what actions will be taken when a new CMake release comes out? What are criteria to bump the |
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. |
|
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. |
|
Note that the latest available CMake version is now also |
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. |
The cmake_minimum_required call does two different things: 1 - It enables & disables various cmake compatibility policies 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.
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. |
|
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 |
CMP0117 stops specifying a redundant rtti flag 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. |
The current CMake code in this project is a bit more complex than necessary. It kind of abuses the Note that "which version is required to make those simplifications" is the wrong question. The correct approach is to set the minimum according to outside requirements (like the version that is available in DebianTesting) and then make simplifications based on that. |
for reference, a bump is done in #175 |
That is great to know. I would love to see a PR simplifying the cmake code here and bumping the minimum version! And that is another reason I would like to merge this PR. Because currently, if a PR simplifying the code and bumping the minimum version was created, it would not only change which cmake versions show fatal minimum version errors, but it would also change a bunch of entirely unrelated build behaviors by altering the policy version as well. This PR prevents that and decouples the policy version from the minimum version. It makes the build use recommended policies instead of deprecated ones, and lets the minimum required version be determined and set independently. |
|
Here is another example of how new policies might be controversial: bitcoin/bitcoin#31504. |
|
Thanks for linking bitcoin/bitcoin#31504. I think it provides another example of why this PR is a good idea. The change in bitcoin/bitcoin#31504 enabling one specific policy turning on link-line deduplication, while leaving all other recommended policies turned off would have added a maintenance burden, and is exactly the type of complexity this PR would avoid. Old linkers may require libraries to be specified multiple lines when linking. Newer linkers do not. Cmake has logic to deal with this complexity and policies which control that logic. The logic was introduced in 3.29 and improved in cmake 3.31. By setting a recent policy version and letting cmake releases actually apply the policies they were designed to use, we stop putting ourselves in a strange no-mans-land where we are using new versions of cmake with old policies. It is true that different versions of cmake can have problems, just like different versions of libraries can have problems, and different versions of compilers can have problems. We should deal with that by pinning dependencies (which we effectively do in guix), not by spending time trying to second-guess cmake's policy decisions and force our users into dozens of deprecated and no longer recommended behaviors. |
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.