This ensures that most compiler tests are not run with the wrong build type’s flags. The initial c++ checks are an exception to that because many internal CMake variables are unset until a language is selected, so it’s problematic to change our build type before that.
The difference can be seen in build/CMakeFiles/CMakeConfigureLog.yaml. Before, Debug was used for many of the earlly checks. After this PR, it’s only the first 2 checks.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31711.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto |
| Concept ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
42+ get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
43+ if(is_multi_config)
44+ get_property(help_string CACHE CMAKE_CONFIGURATION_TYPES PROPERTY HELPSTRING)
45+ set(CMAKE_CONFIGURATION_TYPES "${all_configs}" CACHE STRING "${help_string}" FORCE)
46+ # Also see https://gitlab.kitware.com/cmake/cmake/-/issues/19512.
47+ set(CMAKE_TRY_COMPILE_CONFIGURATION "${config}" PARENT_SCOPE)
CMAKE_TRY_COMPILE_CONFIGURATION variable is responsible for setting the build type in subsequent CMake tests for both single- and multi-config generators.
69+
70+# Set the default config before enabling c++ so the compiler is never run with
71+# the wrong build type's flags.
72+include(DefaultConfig)
73+set_default_config(RelWithDebInfo)
74+
Is the default build configuration really important for detecting the compiler and its capabilities?
UPD. I agree with the changes. Just trying to clarify details.
Tested 240ff782ad5a8974aff35e78356520917926e525 on Ubuntu 24.10 with CMake 3.30.3.
The CMAKE_BUILD_TYPE cache variable lost its help string:
0$ cmake -B build -G "Ninja" -LH 2>&1 | grep -B 1 CMAKE_BUILD_TYPE:
1// Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel ...
2CMAKE_BUILD_TYPE:STRING=RelWithDebInfo
0$ cmake -B build -G "Ninja" -LH 2>&1 | grep -B 1 CMAKE_BUILD_TYPE:
1//
2CMAKE_BUILD_TYPE:STRING=RelWithDebInfo
The
CMAKE_BUILD_TYPEcache variable lost its help string:
Ugh, presumably because it’s enabling c++ that sets this?
I guess that’s a downside, and there may be other side-effects as well. I suppose it’d be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn’t ever be a problem. Agreed?
The
CMAKE_BUILD_TYPEcache variable lost its help string:Ugh, presumably because it’s enabling c++ that sets this?
I guess that’s a downside, and there may be other side-effects as well. I suppose it’d be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn’t ever be a problem. Agreed?
Given #31711 (review), yes, I agree.
No functional change. This is a simple move required the next commit.
I’d like to summarize here a few observations regarding CMake behaviour.
CMAKE_BUILD_TYPE is defined in enable_language().enable_language() ignore any configuration, unless CMAKE_TRY_COMPILE_CONFIGURATION is set beforehand.CMAKE_CONFIGURATION_TYPES is defined in project().enable_language() use the “Debug” configuration (this choice may be generator-specific), unless CMAKE_TRY_COMPILE_CONFIGURATION is set before.Also see: https://gitlab.kitware.com/cmake/cmake/-/issues/26684.
ACK 85c6bafe0b93ca180df3e0ec5c3f1c8d1fefbf9f, tested on Ubuntu 24.10 with the “Ninja” and “Ninja Multi-Config” generators.
A single check still uses the default flags:https://github.com/bitcoin/bitcoin/blob/6a46be75c43e6489d7cb2aaea614ba552b31b35a/cmake/module/TryAppendCXXFlags.cmake#L125
This issue can be addressed with the following patch:
0--- a/cmake/module/ProcessConfigurations.cmake
1+++ b/cmake/module/ProcessConfigurations.cmake
2@@ -4,8 +4,6 @@
3
4 include_guard(GLOBAL)
5
6-include(TryAppendCXXFlags)
7-
8 macro(normalize_string string)
9 string(REGEX REPLACE " +" " " ${string} "${${string}}")
10 string(STRIP "${${string}}" ${string})
11@@ -119,6 +117,8 @@ endfunction()
12
13 set_default_config(RelWithDebInfo)
14
15+include(TryAppendCXXFlags)
16+
17 # We leave assertions on.
18 if(MSVC)
19 remove_cxx_flag_from_all_configs(/DNDEBUG)
With the exception of the first c++ checks, this ensures that compiler tests
are never run with the wrong build type's flags.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This issue can be addressed with the following patch:
Yes, that’s great! Thanks. Applied and added you as co-author.