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

Reviews

See the guideline for information on the review process.

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:

• #33974 (cmake: Check dependencies after build option interaction by hebasto)
• #32367 (cmake: Check user-defined APPEND_*FLAGS variables early by hebasto)

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.

Friendly ping @vasild @fanquake I assume you are interested because of our IRC conversation.

Note: the MSVC check was not tested, I don't have rn an available environment to test it.

Concept ACK

https://github.com/bitcoin/bitcoin/actions/runs/21985815150/job/63519602237?pr=34580#step:9:434:

+ eval 'CMAKE_ARGS=(-DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DCMAKE_INSTALL_PREFIX=/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/out -Werror=dev   --preset=dev-mode   -DWITH_USDT=OFF   -DREDUCE_EXPORTS=ON   -DCMAKE_EXE_LINKER_FLAGS='\''-Wl,-stack_size -Wl,0x80000'\'' )'
++ CMAKE_ARGS=(-DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DCMAKE_INSTALL_PREFIX=/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/out -Werror=dev --preset=dev-mode -DWITH_USDT=OFF -DREDUCE_EXPORTS=ON -DCMAKE_EXE_LINKER_FLAGS='-Wl,-stack_size -Wl,0x80000')
+ cmake -S /Users/runner/work/bitcoin/bitcoin/repo_archive -B /Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/build-aarch64-apple-darwin24.6.0 -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DCMAKE_INSTALL_PREFIX=/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/out -Werror=dev --preset=dev-mode -DWITH_USDT=OFF -DREDUCE_EXPORTS=ON '-DCMAKE_EXE_LINKER_FLAGS=-Wl,-stack_size -Wl,0x80000'
-- The CXX compiler identification is AppleClang 16.0.0.16000026
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:86 (message):
  Clang >= 17.0 required.
-- Configuring incomplete, errors occurred!

Approach ACK a902bd9

I guess the AppleClang needs some exception. What is the deal with it? We support minimum Clang 17 and minimum AppleClang 16? Or is it that the CI uses unsupported compiler (appleclang 16)?

AppleClang 16.0.0.16000026 is based on LLVM 17.0.6, which is OK. That's another reason to test compiler features, not versions.

That's another reason to test compiler fetaures, not versions.

Yea. This was my initial feedback (in IRC) as well.

I'm tending to Concept~0 here. We should be checking features, not version strings that might have exceptions, or be brittle to match against. This might also make it more difficult to test with weird compiler/standard library pairings.

If you want to extract the clang feature check, it is currently in src/util/overloaded.h. See commit faed118

For GCC, I think it is some ranges/view stuff in the kernel tests. The gcc-12 fix from fa3854e was actually backported to gcc-11, so in theory one could use gcc11.3 just fine, if disabling the kernel tests.

I agree that it is more robust to check for features instead of compiler versions. However, then doc/dependencies.md would be at odds with the code - it mentions clang 17 and the code checks for std::xyz availability. Getting them in line would mean rewording doc/dependencies.md to say something like "you need a compiler which supports feature1, feature2 and feature3" - that would not be very useful.

Further, if we check for features, e.g. std::xyz availability and the check fails, then what would be the error message? "find a compiler that supports std::xyz", hmm?

Maybe use a wording like "Need a compiler with support for feature1, feature2, etc, like clang 17 or gcc 12.1" in both doc/dependencies.md and in the error messages?

I think doc/dependencies.md is just the scope for what is tested in CI in this repo and for what users are encouraged to report bugs against. Similar to

Of course, anyone is free to use whatever compiler/OS they want (at their own risk).

It should be trivial to replace requires with is supported and tested with in doc/dependencies.md.

Further, if we check for features, e.g. std::xyz availability and the check fails, then what would be the error message? "find a compiler that supports std::xyz", hmm?

Why not? Maybe the error message can be improved to say "Your current compiler with the current version failed to compile foobar. You may try the next major version of this compiler, or a different supported and tested compiler, according to doc/dependencies.md"

Maybe use a wording like "Need a compiler with support for feature1, feature2, etc, like clang 17 or gcc 12.1" in both doc/dependencies.md and in the error messages?

Seems fine as well, but I don't think there is a need to keep the build system features check in sync with dependencies.md. I think it is enough to have the features check in the build system only.

So, check for features and print a message like "your compiler does not support xyz, Bitcoin Core has been tested to compile with clang >=17 ..."?

Just opened as ready to review to make the CI run and see if the change avoid crashing for Apple.
Will check the check features approach.

$ git diff a902bd955cba72c618269b0481fe6f9d5e23801e..62559d00bdbfa3dd38016d7e7a13df83bba54f0d
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5636f641a0..00676a2ed2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -81,7 +81,7 @@ set(MIN_MSVC_VERSION 19.50)
   # VS 18.x uses 19.50-19.59 as MSVC version values
   # https://cmake.org/cmake/help/latest/variable/MSVC_VERSION.html
 
-if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
   if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS MIN_CLANG_VERSION)
     message(FATAL_ERROR "Clang >= ${MIN_CLANG_VERSION} required.")
   endif()

I've been researching a bit on how to do the features check and I think is not as trivial as I initially thought.

Disclaimer: It's the first time I actually play with CMake files so I don't actually know at all how it works. I've used AI to help me generating the features check CMake file.

From the different options I've seen and tried this approach is the only one that actually worked.

To summarize the solution, cmake will try to compile a piece of code that uses the feature we need. If compilation fails, it returns an error marking the missing feature.

I don't know if there's another cleaner way to check these features. If there is not, I am not sure at all that checking features is a good option, maybe the work to maintaing this extra code and updating it it is not worth at all and just checking compiler versions should be good enough.

At the end the version check that this PR proposes is not restrictive on which compiler to use. It just forces the users, in case they use one of the three "main" compilers that we recommend, to use at least the older known supported version, but it will not fail if a user tries to use a different compiler. The check will just not apply.

Updating that version number should be easy and not much extra work to do in a PR like #33555 and probably is not something that will happen regularly.

Personally if the work to maintain the feature check is considered to be ok and not a big deal I think keeping both approaches, version check (for easy verbose) + feature check (for other compilers) is not a bad idea.

The last commit (760b8c2) adds a feature check following what we have in src/util/overloaded.h.
So using a compiler that does not implement a feature would return something like:

$ cmake -B build -DCMAKE_C_COMPILER=clang-16       -DCMAKE_CXX_COMPILER=clang++-16
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-16 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Checking for required C++20 features...
CMake Error at cmake/module/CheckCXX20Features.cmake:35 (message):
  Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.

  This C++20 feature is required for src/util/overloaded.h.

  Recomended compiler versions:

    - GCC >= 12.1
    - Clang >= 17.0
    - MSVC >= 19.50
Call Stack (most recent call first):
  CMakeLists.txt:97 (check_cxx20_features)


-- Configuring incomplete, errors occurred!

Note: This output appears like this because I dropped the three version check commits before compiling, if the whole PR is tested it will first exit because of the Clang version check. To reach this state without dropping commits, a compiler different from GCC, Clang or MSVC that does not implement CTAD should be used.

Other approaches that I tested and don't work are using target_compile_features.

To clarify the personal motivation for this PR again; this was open after me having an error when building and not understanding why (see IRC logs). As a user not really familiar with compilers and CMake, the output being something like clang min version is 17 and you are using 16 would have avoided me loosing time checking why the code was not compiling and would have avoid me bothering on IRC. Also that message would have been more helpful and straightforward than something like: your compiler doesn't implement xyz. At that moment I don't really care about the features, I just need the code to compile so the version check tells me update the compiler and I will work which is exactly what I needed at that moment.

Again I am not really familiar with CMake and maybe I am missing some cleaner way to do this feature check, help, and approach ideas are more than welcome :)

This might also make it more difficult to test with weird compiler/standard library pairings.

This doesn't force users to use a specific compiler, only forces to, if you used Clang, GCC or MSVC, use at least the older required version. But users can still use weird compilers and this checks will be skipped.

ACK 1ccc075 (full PR) ACK 91e64c9 (just first two commits)

Would be nice to get some feedback from @hebasto, @fanquake, @sedited and @maflcko - are the first two commits what you liked to see (checking for features, instead of compiler version numbers)?

I've already provided my feedback in #34580 (comment), but I haven't seen a reply so far, so I am not sure what else I can do here?

I've already provided my feedback in #34580 (comment), but I haven't seen a reply so far, so I am not sure what else I can do here?

@maflcko in order to address the feedback features check was implemented too. The order of the commits was also changed to make it easy to only review the features check.

Feel free to only review the first two commits and NACK or ignore the last three. If there's consensus among the reviewers, I will be happy to drop the last commits (version check).

Force pushed to fix two small comments:

$ git diff 91e64c95116d4d1ca3f7176a9d83dcc339c4481b..eca0c31c130c3deb68acd98325580052950981b5
diff --git a/cmake/module/CheckCXXFeatures.cmake b/cmake/module/CheckCXXFeatures.cmake
index c1f91d79d8..9ebb769104 100644
--- a/cmake/module/CheckCXXFeatures.cmake
+++ b/cmake/module/CheckCXXFeatures.cmake
@@ -2,6 +2,8 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or https://opensource.org/license/mit/.
 
+include_guard(GLOBAL)
+
 #Checks for C++ features required to compile Bitcoin Core.
 
 include(CheckCXXSourceCompiles)
@@ -30,7 +32,7 @@ function(check_cxx_features)
     message(FATAL_ERROR
       "Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.\n"
       "This C++ feature is required for src/util/overloaded.h.\n"
-      "Recomended compiler versions:\n"
+      "Recommended compiler versions:\n"
       "  - GCC >= ${MIN_GCC_VERSION}\n"
       "  - Clang >= ${MIN_CLANG_VERSION}\n"
       "  - MSVC >= ${MIN_MSVC_VERSION}"

Given the feedback and lack of support for version check, I force pushed to drop the version check and keep only the feature checks.

Looks like the version checking commit is still here?

Looks like the version checking commit is still here?

@fanquake The version check was dropped. The first commit only sets some variables used for suggesting versions to the user in case the feature check is not passed.

I have just updated the commit message to make it more clear.

Force pushed to solve some nits.

Also changed the output from printing the recommended version to print the path to the compiler requirements documentation. This way we avoid the need to update the value if the requirements change and avoid silent conflicts with other PRs #31507