build: Make config warnings fatal if -DWERROR=ON

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 core repo) 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) But this has not resulted in CI failure, even though utils and rpcauth tests have been skipped. Making this matter a bit worse, we now also use a match statement in test/functional/combine_logs.py, which results in a SyntaxError since match was introduced in python 3.10, I am not sure that this is all that important right now since combine_logs.py only gets run if a functional test fails.

This branch makes the presence of configure_warnings a FATAL_ERROR if -DWERROR=ON, it also makes an incompatible BDB version a configure_warning if -DWARN_INCOMPATIBLE_BDB=ON.

This depends on #31593 which bumps CentOS to Stream 10, so marking it as draft for now.

Slightly outside the scope of this PR, but maybe not a big enough question to deserve its own issue: 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?

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#31359 (cmake: Add CheckLinkerSupportsPIE module by hebasto)
#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

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.

build: reuse configure_warnings for bdb warning. 8bef4ae64f

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:658 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

build: Make config warnings fatal if -DWERROR=ON

Also log `configure_warnings` to cmake-configure-log before a
potentially fatal message to make CI output more readable.

587f53e7b8

davidgumberg force-pushed on Jan 17, 2025

in CMakeLists.txt:84 in 587f53e7b8

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.

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts