libmultiprocess
libmultiprocess copied to clipboard
chaincodelabs
Set cmake_minimum_required(VERSION 3.22)
This PR is doing two different things:
- It is switching on cmake policies CMP0076-CMP0128
- It is triggering a fatal error if versions of cmake before 3.22 are used.
This is a follow-up to the last bump in https://github.com/bitcoin-core/libmultiprocess/pull/164.
It is unclear why effort should be made to try to support earlier cmake versions than the ones Bitcoin Core is using (https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/CMakeLists.txt#L10), because libmultiprocess is currently only used there.
Moreover, old systems shipping with ancient cmake versions are likely already incompatible due to https://github.com/bitcoin-core/libmultiprocess/pull/205, if they lack the appropriate point release of capnproto.
Once there is a need or use-case to derive from Bitcoin Core, the policy could be changed then.
Bumping the minimum could also unlock simpler cmake code, see https://github.com/bitcoin-core/libmultiprocess/pull/163#issuecomment-2886577273
Concept ~0 assuming #163 is merged first.
This PR is doing two different things (see https://github.com/bitcoin-core/libmultiprocess/pull/163#issuecomment-2884186860)
- It is switching on cmake policies CMP0076-CMP0128
- It is triggering a fatal error if versions of cmake before 3.22 are used.
I think the first change is good. The second change seems a like it introduces misleading documentation and an inappropriate error message, but it is not very damaging and seems ok if we want to be paranoid about old versions of cmake doing bad things.
I would like to see #163 merged first because it is only doing the first thing, not the second thing and I do think it is good to keep these things separate for clarity.
This PR is doing two different things
correct. Thanks for making it explicit. I like both things and I think it is the most simple and most practical way forward for now.
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 | hebasto, fanquake |
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:
- #212 (ci: add newdeps job testing newer versions of cmake and capnproto by ryanofsky)
- #209 (cmake: Increase cmake policy version by ryanofsky)
- #163 (build: set cmake policy version to 3.31 by purpleKarrot)
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.
For reference, the approach here is also taken by other software projects, like Google-OSS: https://github.com/google/crc32c/pull/68, https://opensource.google/documentation/policies/cplusplus-support
ACK 3f386cc3e31bd049eb0d069644a4d7188053b4bb - just because it would now match what Core is setting, which seems reasonable.
(Rebased for silent merge conflict. Also, removed now unused ci code. Should be trivial to re-review.)
I'm not sure what to do about this PR because I feel like it is not describing itself or the code accurately.
From the PR description it sounds like it is only dropping support for old versions of cmake but this is not the case, since the changing poilcy version affects behavior of all versions of cmake including the newest ones. I don't think there is a good reason to tie changes to the policy version and minimum version together, because they are separate concepts and have different effects.
Writing cmake_minimum_required(VERSION 3.22) also seems misleading because it implies the cmake build requires features present in 3.22, which it doesn't. This could be fixed by adding a comment, though.