Set cmake_minimum_required(VERSION 3.22)

[maflcko]

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 #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 #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 #163 (comment)

[hebasto]

ACK 3f386cc.

[ryanofsky]

Concept ~0 assuming #163 is merged first.

This PR is doing two different things (see #163 (comment))

1. It is switching on cmake policies CMP0076-CMP0128
2. 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.

[maflcko]

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.

[DrahtBot]

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 copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #274 (Add nonunix platform support by ryanofsky)
• #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.

[maflcko]

For reference, the approach here is also taken by other software projects, like Google-OSS: google/crc32c#68, https://opensource.google/documentation/policies/cplusplus-support

[fanquake]

ACK 3f386cc - just because it would now match what Core is setting, which seems reasonable.

[hebasto]

ACK 34f48d1.

[maflcko]

(Rebased for silent merge conflict. Also, removed now unused ci code. Should be trivial to re-review.)

[fanquake]

ACK 34f48d1

[ryanofsky]

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.

[maflcko]

thx, adjusted pull description (first section)

[DrahtBot]

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it with one of the labels 'Up for grabs' or 'Insufficient Review', so that it can be picked up in the future.

[maflcko]

The current status is that there are two reviews, and a suggestion to update the description, which I've done.

[maflcko]

This has two acks and no open feedback, so I wonder if there is a blocker before merge?

in the meantime, the leveldb subtree also has this set:

diff --git a/src/leveldb/CMakeLists.txt b/src/leveldb/CMakeLists.txt
index cfd4faa325..78efde95de 100644
--- a/src/leveldb/CMakeLists.txt
+++ b/src/leveldb/CMakeLists.txt
@@ -2,19 +2,25 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file. See the AUTHORS file for names of contributors.
 
-cmake_minimum_required(VERSION 3.9)
+cmake_minimum_required(VERSION 3.22)

[ryanofsky]

This has two acks and no open feedback, so I wonder if there is a blocker before merge?

Thanks for following up. I haven't merged this PR or known how to give more constructive feedback about it, because I'm still confused about the motivation behind it. The PR description has improved since the last time I saw it, but it is only offering rationalizations about why these changes are not harmful, not explaining why these changes are actually good or desirable.

From my perspective, the PR is making two distinct changes that are confusing to bundle together, and one of them seems bad (breaks working builds and misleadingly documents build requirements), and other one might be good, but the thinking behind it is not explained (it chooses a newer policy version number but doesn't explain how the number was chosen, or how the number should be chosen in the future, why cmake developer guidelines are ignored, or what the tradeoffs are).

Since I don't understand the motivation behind this PR, I'm not sure I can make a suggestion that would accomplish whatever goals it is trying to accomplish. But I don't think I'd have any issue with this if were limited to just raising the policy version, since I think everybody agrees policy version is probably lower than it should be.

[maflcko]

The motivation boils down to this in the pull description: "It is unclear why effort should be made to try to support earlier cmake versions".

That is, I think there is no value in writing extra code (even if it is "only" CI code) to try to support ancient cmake versions, where it is known that no user is running them.

I don't really see an alternative here. One could just commit the ci/scripts/ci.sh changes, and a ci-only and nix-only bump to 3.15, but then CMakeLists.txt would contain an untested cmake_minimum_required, so I think all the changes should go together. The only alternative I see here is using 3.15 instead of 3.22, to do the minimal bump needed to be able to drop the extra code.

Though, that seems backward. The correct approach is to pick the version, based on outside requirements and then apply the simplifications afterward, see the second section of #163 (comment).

[ryanofsky]

re: #175 (comment)

I do not mean to be uncharitable, but this response seems to be full of false assumptions.

There is no "correct approach" here, there are tradeoffs between different approaches.

There is no "extra code" here, there is just code that supports different use-cases.

The claim that "no user is running [the current minimum required version]" is not true because I am a user and I have a branch and PR testing that version. I also view code as documentation and prefer code that documents true things ("X is required") instead of false things ("X is not required but if we pretend it is, the simplest part of CI system can become slightly simpler".)

The sentence "I don't really see an alternative here" seems misguided because this PR is not solving any current problem.

I am guessing we have different viewpoints because we have different backgrounds. I think you are more used to a debian model where an upstream provider chooses decides which versions of packages should be used together and makes customization relatively difficult. My background is using configure/make/install, then gentoo, then nix, which all make it easy mix and match versions and apply customization of different kinds.

If the goal of changing the minimum version and policy version together is just to simplify the CI scripts slightly, when the most complicated part of the CI system by far is the sprawling YAML configuration, not the 20 line testing script, then I'd suggest closing this PR. It is not worth falsely stating requirements or changing the policy version (which affects builds in a myriad of ways) to do this.

I'm completely happy with raising the minimum version when there is a real reason to raise it, but slightly simplifying a CI script doesn't seem like a real reason given that it changes which build policies are used in non-CI builds, breaks currently working builds, and misstates requirements.

[maflcko]

the most complicated part of the CI system by far is the sprawling YAML configuration

I am happy to review a pull request simplifying this. Feel free to use an LLM bot for this. I think most of this can be simplified, according to #253 (comment). Would be interesting to see if there are any obstacles.

I'd suggest closing this PR

Sure, if it is too subjective/controversial. I'd presume the same change change will be picked up later when this is bumped to cmake 3.23 (or later) when https://cmake.org/cmake/help/latest/command/target_sources.html#file-sets FILE_SETs are made use of.