build: Make config warnings fatal if -DWCONFIGURE

davidgumberg commented at 11:04 pm on January 15, 2025: contributor

Summarizing #31476, the CentOS CI task has been configuring a build with python 3.9 (the latest version in the Stream 9 BaseOS repository) which is below the minimum version of 3.10 which resulted in a warning being appended to the configure_warnings cmake variable. https://github.com/bitcoin/bitcoin/blob/712cab3a8f8ad76db959337ddc35cb4c34cac388/CMakeLists.txt#L546-L553

This warning has been emitted by every CI run on CentOS¹ 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.

e.g. https://cirrus-ci.com/task/5526283183980544?logs=ci#L895, https://cirrus-ci.com/task/6597682371756032?logs=ci#L894 ↩︎

DrahtBot commented at 11:04 pm on January 15, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31665.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	purpleKarrot
Stale ACK	s373nZ

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:

#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)
#31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
#30595 (kernel: Introduce initial C header API by TheCharlatan)

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

Related to #31476.

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

Related to #31476.

🤦 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:687 in 2b2c9e6416 outdated

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.

hebasto commented at 8:48 pm on January 16, 2025:

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

in CMakeLists.txt:92 in 587f53e7b8 outdated

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)

hebasto commented at 9:03 am on January 17, 2025:

style nit: 2 space indentation.

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()
18 
19 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).

Making this matter a bit worse, we now also use a match statement

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.

maybe this discussion is more relevant in #31669

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.

Making this matter a bit worse, we now also use a match statement

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

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.

0ddd45fd70

build: reuse configure_warnings for bdb warning. bb7a683b69

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

ACK https://github.com/bitcoin/bitcoin/pull/31665/commits/eaf6a5198d2b9033a6d6f0a4c01faddc67152857

Tested generation under the following variety of option combinations on Ubuntu 24.04.2 LTS:

libdb++dev package not installed: cmake -B build -DWCONFIGURE_ERROR=ON -DWARN_INCOMPATIBLE_BDB=ON -DENABLE_WALLET=ON -DWITH_BDB=ON --fresh

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)

libdb++dev package installed (5.3): cmake -B build -DWCONFIGURE_ERROR=ON -DWARN_INCOMPATIBLE_BDB=ON -DENABLE_WALLET=ON -DWITH_BDB=ON --fresh

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.
3
4  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".
3
4  Passing "-DWITH_BDB=OFF" will suppress this warning.
5
6
7-- Configuring incomplete, errors occurred!

libdb++dev package installed (5.3): cmake -B build -DWCONFIGURE_ERROR=ON -DWARN_INCOMPATIBLE_BDB=OFF -DENABLE_WALLET=ON -DWITH_BDB=ON --fresh

0CMake Warning at CMakeLists.txt:123 (message):
1  Found Berkeley DB (BDB) other than 4.8.
2
3  BDB (legacy) wallets opened by this build will not be portable!

followed by

0-- Configuring done (11.4s)
1-- Generating done (0.1s)

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.

For example:

0cmake_minimum_required(VERSION 3.10)
1project(min)
2
3message(WARNING "Warn")

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
3
4
5-- 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

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.

dc2a806f46

ci: Enable WCONFIGURE_ERROR in ci jobs 060101b19a

guix: -DWCONFIGURE_ERROR=ON for guix builds 3e56d5b111

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.

build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON #31665

Code Coverage & Benchmarks

Reviews

Conflicts