This warning has been emitted by every CI run on CentOS1 since the minimum version was bumped (#30527) and until the centos task was bumped to stream 10 in #31593 But this has not resulted in CI failure, even though utils and rpcauth tests have been skipped.
This branch adds a cmake build option WCONFIGURE_ERROR that if on makes the presence of configure_warnings a FATAL_ERROR it also makes an incompatible BDB version a configure_warning if -DWARN_INCOMPATIBLE_BDB=ON.
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:
#32367 (cmake: Check user-defined APPEND_*FLAGS variables early by hebasto)
#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
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.
DrahtBot added the label
Build system
on Jan 15, 2025
hebasto
commented at 8:39 am on January 16, 2025:
member
fanquake
commented at 11:45 am on January 16, 2025:
member
Assuming the CI is always configured to use -DWERROR, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it’s hidden far back in the scrollback.
fanquake
commented at 11:48 am on January 16, 2025:
member
if we start using syntax features and behavior that are only available in versions greater than the minimum?
This should never be the case, otherwise the minimum required version isn’t actually the minimum required version.
davidgumberg force-pushed
on Jan 16, 2025
davidgumberg
commented at 6:54 pm on January 16, 2025:
contributor
🤦 I did not realize that’s what #31593 was fixing.
Assuming the CI is always configured to use -DWERROR, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it’s hidden far back in the scrollback.
Done, by logging configure_warnings to the cmake-configure-log.
if we start using syntax features and behavior that are only available in versions greater than the minimum?
This should never be the case, otherwise the minimum required version isn’t actually the minimum required version.
Sorry, I misspoke, I meant syntax features and behavior that are only available in the minimum version or greater. Anyways, I think it’s fine to leave it as a warning, since it doesn’t impact the executables.
davidgumberg force-pushed
on Jan 16, 2025
DrahtBot added the label
CI failed
on Jan 16, 2025
in
CMakeLists.txt:727
in
2b2c9e6416outdated
654@@ -650,7 +655,8 @@ message("\n")
655 if(configure_warnings)
656 message(" ******\n")
657 foreach(warning IN LISTS configure_warnings)
658- message(WARNING "${warning}")
659+ message(CONFIGURE_LOG "${warning}") # Log to the cmake-configure-log file before a potentially fatal message.
message(CONFIGURE_LOG ...) was added in CMake version 3.26. Our minimum required one is 3.22.
davidgumberg
commented at 2:06 am on January 17, 2025:
Thanks, fixed.
davidgumberg force-pushed
on Jan 17, 2025
davidgumberg force-pushed
on Jan 17, 2025
in
CMakeLists.txt:92
in
587f53e7b8outdated
75@@ -76,6 +76,14 @@ list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)
76 #=============================
77 include(CMakeDependentOption)
78 # When adding a new option, end the <help_text> with a full stop for consistency.
79+set(configure_warnings)
80+option(WERROR "Treat compiler warnings as errors." OFF)
81+if(WERROR)
82+ set(configure_warning_message_type FATAL_ERROR)
83+else()
84+ set(configure_warning_message_type WARNING)
davidgumberg
commented at 4:45 pm on February 19, 2025:
Thanks, fixed
hebasto
commented at 9:06 am on January 17, 2025:
member
I don’t like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
However, if this PR proceeds in its current direction, the new functionality must be clearly documented in the WERROR build option’s help string.
Although one thing that could be improved is making the error more clear, as in the current output, it’s hidden far back in the scrollback.
There is no need to continue after encountering an error:
0--- a/CMakeLists.txt
1+++ b/CMakeLists.txt
2@@ -78,11 +78,13 @@ include(CMakeDependentOption)
3 # When adding a new option, end the <help_text> with a full stop for consistency.
4 set(configure_warnings)
5 option(WERROR "Treat compiler warnings as errors." OFF)
6-if(WERROR)
7- set(configure_warning_message_type FATAL_ERROR)
8-else()
9- set(configure_warning_message_type WARNING)
10-endif()
11+macro(user_message message)
12+ if(WERROR)
13+ message(FATAL_ERROR ${message})
14+ else()
15+ list(APPEND configure_warnings ${message})
16+ endif()
17+endmacro()
1819 option(BUILD_DAEMON "Build bitcoind executable." ON)
20 option(BUILD_GUI "Build bitcoin-qt executable." OFF)
21@@ -116,7 +118,7 @@ if(WITH_BDB)
22 "BDB (legacy) wallets opened by this build will not be portable!"
23 )
24 if(WARN_INCOMPATIBLE_BDB)
25- list(APPEND configure_warnings
26+ user_message(
27 "Incompatible Berkeley DB found. If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\nPassing \"-DWITH_BDB=OFF\" will suppress this warning."
28 )
29 endif()
30@@ -552,7 +554,7 @@ find_package(Python3 3.10 COMPONENTS Interpreter)
31 if(Python3_EXECUTABLE)
32 set(PYTHON_COMMAND ${Python3_EXECUTABLE})
33 else()
34- list(APPEND configure_warnings
35+ user_message(
36 "Minimum required Python not found. Utils and rpcauth tests are disabled."
37 )
38 endif()
39@@ -655,10 +657,7 @@ message("\n")
40 if(configure_warnings)
41 message(" ******\n")
42 foreach(warning IN LISTS configure_warnings)
43- if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.26)
44- message(CONFIGURE_LOG "${warning}") # Log to the cmake-configure-log file before a potentially fatal message.
45- endif()
46- message(${configure_warning_message_type} "${warning}")
47+ message(WARNING "${warning}")
48 endforeach()
49 message(" ******\n")
50 endif()
fanquake
commented at 9:34 am on January 17, 2025:
member
I don’t like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
Can you elaborate on why this would be unexpected? This is an option to turn warnings into errors, and it’d just be turning more warnings into errors. I’m assuming any user who’d want to use this locally is exactly the kind of user who’d rather see more warnings than less?
hebasto
commented at 11:19 am on January 17, 2025:
member
I don’t like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
Can you elaborate on why this would be unexpected? This is an option to turn warnings into errors, and it’d just be turning more warnings into errors.
Other projects use the same or similar build option only to turn compiler’s warnings into errors.
fanquake
commented at 1:46 pm on January 17, 2025:
member
Other projects use the same or similar build option only to turn compiler’s warnings into errors.
Sure, but should that dictate what we do? Having an option to turn warnings into errors, that turns slightly more warnings into errors compared to other projects, and is mostly used in our CI where we want to surface all warnings, seems reasonable. Given that CMake lacks any native functionality to acheive the same thing, using -DWERROR seems ok, especially if the alternative is implement N more build options (now/in future) with varying defaults for everything that may warn and needs to be caught.
luke-jr
commented at 2:40 pm on January 23, 2025:
member
should failing to meet the minimum python version just be a warning if we start using syntax features and behavior that are only available in versions equal to or greater than the minimum?
The minimum should probably not be bumped at all, until we are ready to start using features requiring it… and that usage is why we’re disabling tests if the minimum isn’t met, right?
hebasto
commented at 2:58 pm on January 23, 2025:
member
As pointed in #31724 (comment), this change will break builds on systems where a toolchain does not support PIE.
maflcko
commented at 3:37 pm on January 23, 2025:
member
Summarizing #31476, the CentOS CI task has been configuring a build with python 3.9 (the latest version in the Stream 9 core repo) which is below the minimum version of 3.10
I don’t think this is entirely right. appstream ships with 3.11 and 3.12 on centos. (You can install them just like you can install g++-14 on centos 9).
I don’t think this is “worse”. If the minimum version is 3.10, then any feature in that version should be allowed to be used.
For context, the bump was done for two reasons:
To allow new features (like the match statement)
Because no CI task was presumed to be running under 3.9 anyway (this turned out to be wrong, but now that the centos task is bumped, this is now true)
davidgumberg force-pushed
on Feb 8, 2025
davidgumberg marked this as ready for review
on Feb 8, 2025
davidgumberg force-pushed
on Feb 8, 2025
davidgumberg
commented at 3:08 am on February 8, 2025:
contributor
Given that CMake lacks any native functionality to acheive the same thing, using -DWERROR seems ok, especially if the alternative is implement N more build options (now/in future) with varying defaults for everything that may warn and needs to be caught.
I don’t like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
As pointed in #31724 (comment), this change will break builds on systems where a toolchain does not support PIE.
In light of the fact that that reusing WERROR would make it impossible to build with WERROR on a platform that does not support PIE, and to avoid overloading WERROR with other unexpected behavior while maintaining a general solution, I’ve added 1 more build flag WCONFIGURE_ERROR which if on, makes the presence of configure_warnings an error, and enabled it in CI jobs.
I don’t think this is “worse”. If the minimum version is 3.10, then any feature in that version should be allowed to be used.
The minimum should probably not be bumped at all, until we are ready to start using features requiring it… and that usage is why we’re disabling tests if the minimum isn’t met, right?
What I was trying to get at is:
Why maintain a list of tests that get disabled if the python version is below the minimum, if this list has not been maintained as newer python features have been used elsewhere. (e.g. match statement in combine_logs.py) and if the actual version of python required to run these tests is not even the minimum? (python 3.9, the minimum when the check was introduced)
It seems that either trying to run the functional tests with ctest should fail if the python version is below the minimum, or it should just proceed at the peril of the user, but I don’t understand the present state where these tests are silently disabled.
I’ve edited the PR description to better phrase this question since I think the way that I worded it was confusing or didn’t make sense.
davidgumberg renamed this:
build: Make config warnings fatal if -DWERROR=ON
build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON
on Feb 8, 2025
DrahtBot removed the label
CI failed
on Feb 8, 2025
davidgumberg force-pushed
on Feb 10, 2025
davidgumberg
commented at 11:28 pm on February 10, 2025:
contributor
Changed the test each commit CI job to use -DWARN_INCOMPATIBLE_BDB=OFF, otherwise it results in a fatal error if -DWCONFIGURE_ERROR=ON.
davidgumberg
commented at 2:16 pm on February 11, 2025:
contributor
Summarizing #31476, the CentOS CI task has been configuring a build with python 3.9 (the latest version in the Stream 9 core repo) which is below the minimum version of 3.10
I don’t think this is entirely right. appstream ships with 3.11 and 3.12 on centos. (You can install them just like you can install g++-14 on centos 9).
By ‘core repo’ I meant what I guess CentOS calls the ‘BaseOS’ repository, fixed in the description.
I don’t think this is “worse”. If the minimum version is 3.10, then any feature in that version should be allowed to be used.
I was not trying to say that it is a bad thing that there are features from the minimum version being used, I was trying to motivate failure to meet minimum python requirements deserving stricter enforcement than a warning, but since this and the other question I asked about making a Python version below the minimum an error have been poorly communicated by me and slightly off-topic, I’ve removed them from the PR description.
DrahtBot added the label
Needs rebase
on Feb 14, 2025
davidgumberg force-pushed
on Feb 14, 2025
davidgumberg
commented at 8:32 pm on February 14, 2025:
contributor
Rebased to fix merge conflict.
DrahtBot removed the label
Needs rebase
on Feb 14, 2025
s373nZ
commented at 1:27 pm on February 18, 2025:
none
0CMake Error at /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
1 Could NOT find BerkeleyDB (missing: BerkeleyDB_LIBRARY
2 BerkeleyDB_INCLUDE_DIR)(Required is at least version "4.8")3Call Stack (most recent call first):
4 /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)5 cmake/module/FindBerkeleyDB.cmake:107 (find_package_handle_standard_args)6 CMakeLists.txt:120 (find_package)
0-- Found BerkeleyDB: /usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so (found suitable version "5.3.28", minimum required is "4.8")1CMake Warning at CMakeLists.txt:123 (message):
2 Found Berkeley DB (BDB) other than 4.8.
34 BDB (legacy) wallets opened by this build will not be portable!
followed by
0CMake Error at CMakeLists.txt:689 (message):
1 Incompatible Berkeley DB found. If this is intended, pass
2 "-DWARN_INCOMPATIBLE_BDB=OFF".
34 Passing "-DWITH_BDB=OFF" will suppress this warning.
567-- Configuring incomplete, errors occurred!
0CMake Warning at CMakeLists.txt:123 (message):
1 Found Berkeley DB (BDB) other than 4.8.
23 BDB (legacy) wallets opened by this build will not be portable!
libdb++dev package not installed, use depends toolchain: cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DWCONFIGURE_ERROR=ON -DWARN_INCOMPATIBLE_BDB=ON -DENABLE_WALLET=ON -DWITH_BDB=ON --fresh
No warnings, no errors.
Although the goal with this flag is toward stricter CI, these tests explore behavior under some general conditions.
maflcko
commented at 2:23 pm on February 18, 2025:
member
No opinion about the changes here, but if they are done, shouldn’t the deterministic guix release build also set DWCONFIGURE_ERROR=ON? If there are any errors, it seems better to explicitly silence them.
purpleKarrot
commented at 7:25 am on February 19, 2025:
contributor
Concept NACK.
Please don’t add nonstandard cmake variables (here: WCONFIGURE_ERROR=ON) for things that have builtin support in CMake. Just issue warnings with the message command:
0message(WARNING"There is some potential issue on the system, like a new version of a dependency that has not been tested yet.")1message(AUTHOR_WARNING"There is a potention issue in cmake code, like suspicious arguments to a custom function")2message(DEPRECATION"You may be calling a custom function that is planned for removal, please migrate")
Clients can then control via command line flags like -Werror=<what> what warnings they want to see hand whether they should be interpreted as error.
davidgumberg
commented at 8:20 am on February 19, 2025:
contributor
Clients can then control via command line flags like -Werror=<what> what warnings they want to see hand whether they should be interpreted as error.
Thanks for the suggestion, it seems like cmake’s -Werror=<what> does not support making WARNING messages errors, only AUTHOR_WARNING with -Werror=dev and DEPRECATION with -Werror=deprecated.
Running cmake configuration with this example and -Werror=dev set succeeds without error:
0$ cmake -B build -Werror=dev
1CMake Warning at CMakeLists.txt:4 (message):
2 Warn
345-- Configuring done (0.0s)
6-- Generating done (0.0s)
7-- Build files have been written to: /tmp/warningexample/build
davidgumberg force-pushed
on Feb 19, 2025
davidgumberg force-pushed
on Feb 19, 2025
davidgumberg
commented at 4:49 pm on February 19, 2025:
contributor
Rebased to address reviewer feedback that the guix builds should have this option enabled, and that CMakeLists.txt uses 2 spaces for indentation, not 4.
DrahtBot added the label
Needs rebase
on Mar 14, 2025
ci: -DWARN_INCOMPATIBLE=OFF in each-commit job
This is necessary since Ubuntu packages libdb 5.x, and the test results
in a configuration warning currently.
f1bb425ed8
build: reuse configure_warnings for bdb warning.0d863f984d
build: WCONFIGURE_ERROR makes config errors fatal.
Also log `configure_warnings` to cmake-configure-log before a
potentially fatal message to make CI output more readable.
d13d7e4f41
ci: Enable WCONFIGURE_ERROR in ci jobs3064859a3b
guix: -DWCONFIGURE_ERROR=ON for guix buildsdf601318ae
davidgumberg force-pushed
on May 6, 2025
davidgumberg
commented at 6:51 pm on May 6, 2025:
contributor
Rebased to fix merge conflict, this needs conceptual review, it seems to me the main question is: Should it be possible to make config warnings fatal? Or is it the case that any time you want to make a warning fatal, you have a warning that should probably always be an error?
It’s possible also that having a python version below the minimum should always be an error, and that some other warnings should be configurably errorized.. . I think it’s worth separating out the python version warning issue from the general issue of #31476.
DrahtBot removed the label
Needs rebase
on May 6, 2025
DrahtBot added the label
Needs rebase
on May 7, 2025
DrahtBot
commented at 2:25 pm on May 7, 2025:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-12 00:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me