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

davidgumberg · 2025-01-15T23:04:54Z

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.

bitcoin/CMakeLists.txt

Lines 546 to 553 in 712cab3

    
           find_package(Python3 3.10 COMPONENTS Interpreter) 
        
           if(Python3_EXECUTABLE) 
        
             set(PYTHON_COMMAND ${Python3_EXECUTABLE}) 
        
           else() 
        
             list(APPEND configure_warnings 
        
               "Minimum required Python not found. Utils and rpcauth tests are disabled." 
        
             ) 
        
           endif()

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 · 2025-01-15T23:04:57Z

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:

#32865 (cmake: Use AUTHOR_WARNING for warnings by fanquake)
#31507 (build: Use clang-cl to build on Windows natively 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.

hebasto · 2025-01-16T08:39:26Z

Related to #31476.

fanquake · 2025-01-16T11:45:23Z

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 · 2025-01-16T11:48:21Z

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 · 2025-01-16T18:54:18Z

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.

CMakeLists.txt

hebasto

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:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -78,11 +78,13 @@ include(CMakeDependentOption)
 # When adding a new option, end the <help_text> with a full stop for consistency.
 set(configure_warnings)
 option(WERROR "Treat compiler warnings as errors." OFF)
-if(WERROR)
-    set(configure_warning_message_type FATAL_ERROR)
-else()
-    set(configure_warning_message_type WARNING)
-endif()
+macro(user_message message)
+  if(WERROR)
+    message(FATAL_ERROR ${message})
+  else()
+    list(APPEND configure_warnings ${message})
+  endif()
+endmacro()
 
 option(BUILD_DAEMON "Build bitcoind executable." ON)
 option(BUILD_GUI "Build bitcoin-qt executable." OFF)
@@ -116,7 +118,7 @@ if(WITH_BDB)
                     "BDB (legacy) wallets opened by this build will not be portable!"
     )
     if(WARN_INCOMPATIBLE_BDB)
-      list(APPEND configure_warnings
+      user_message(
         "Incompatible Berkeley DB found. If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\nPassing \"-DWITH_BDB=OFF\" will suppress this warning."
       )
     endif()
@@ -552,7 +554,7 @@ find_package(Python3 3.10 COMPONENTS Interpreter)
 if(Python3_EXECUTABLE)
   set(PYTHON_COMMAND ${Python3_EXECUTABLE})
 else()
-  list(APPEND configure_warnings
+  user_message(
     "Minimum required Python not found. Utils and rpcauth tests are disabled."
   )
 endif()
@@ -655,10 +657,7 @@ message("\n")
 if(configure_warnings)
     message("  ******\n")
     foreach(warning IN LISTS configure_warnings)
-      if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.26)
-        message(CONFIGURE_LOG "${warning}") # Log to the cmake-configure-log file before a potentially fatal message.
-      endif()
-      message(${configure_warning_message_type} "${warning}")
+      message(WARNING "${warning}")
     endforeach()
     message("  ******\n")
 endif()

CMakeLists.txt

fanquake · 2025-01-17T09:34:07Z

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 · 2025-01-17T11:19:22Z

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 · 2025-01-17T13:46:26Z

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 · 2025-01-23T14:40:52Z

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 · 2025-01-23T14:58:03Z

As pointed in #31724 (comment), this change will break builds on systems where a toolchain does not support PIE.

maflcko · 2025-01-23T15:37:57Z

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 · 2025-02-08T03:08:12Z

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 · 2025-02-19T16:49:05Z

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.

davidgumberg · 2025-05-06T18:51:53Z

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.

davidgumberg · 2025-05-29T05:46:11Z

Rebased to address merge conflict after legacy wallet removal.

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

.github/ci-test-each-commit-exec.sh

fanquake · 2025-07-03T16:40:33Z

@davidgumberg FYI I've implemented something using AUTHOR_WARNING, as discussed above, in #32865.

davidgumberg · 2025-07-03T17:21:34Z

OK, closing this in favor of #32865.

DrahtBot · 2025-07-03T18:56:14Z

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit `49d5f1f` (master)	commit `9d350ae` (pull/31665/merge)
*-aarch64-linux-gnu-debug.tar.gz	`057faa2959973d63...`	`473df5425acdf4c8...`
*-aarch64-linux-gnu.tar.gz	`c97e8a1c0cc1113a...`	`dc190dc186089437...`
*-arm-linux-gnueabihf-debug.tar.gz	`a8ae8620c87df297...`	`c01dcb9eb16cbb33...`
*-arm-linux-gnueabihf.tar.gz	`9c95feadb2cd3d8c...`	`691158caf1e85cfb...`
*-arm64-apple-darwin-codesigning.tar.gz	`2aeee15ab8fefff5...`	`e2fa5f24d44063fe...`
*-arm64-apple-darwin-unsigned.tar.gz	`b060907cd93b0e2f...`	`c27466c1c1ddfe75...`
*-arm64-apple-darwin-unsigned.zip	`ee16ad0c68370e4b...`	`c92d2f0d5426db49...`
*-powerpc64-linux-gnu-debug.tar.gz	`4fa32cd3ec3097be...`	`00d0480c3141eeef...`
*-powerpc64-linux-gnu.tar.gz	`42e0dee2ca0d6498...`	`fb4856f070356a5a...`
*-riscv64-linux-gnu-debug.tar.gz	`e11517dec6cb6d34...`	`d828a20fbdefbcd2...`
*-riscv64-linux-gnu.tar.gz	`2f795a11676010ee...`	`2566c131943fd82a...`
*-x86_64-apple-darwin-codesigning.tar.gz	`adb7b75317494139...`	`c78822586a3467ed...`
*-x86_64-apple-darwin-unsigned.tar.gz	`982cfa7fac715c92...`	`b402e8457def872e...`
*-x86_64-apple-darwin-unsigned.zip	`6230b5feab5dafe9...`	`56626e1c1b9df208...`
*-x86_64-linux-gnu-debug.tar.gz	`6c7da59370125845...`	`d205be2c09c4e9b6...`
*-x86_64-linux-gnu.tar.gz	`12c383385aa98745...`	`6d8f77cc6986d8ca...`
*.tar.gz	`e101268a322f4f9d...`	`6cd671d10441b023...`
SHA256SUMS.part	`e1ff72f28a372cf1...`	`9bbeb9f2b140507c...`
guix_build.log	`b607a09532e808a9...`	`df16901050aec393...`
guix_build.log.diff		`7acda06013b9823f...`

DrahtBot added the Build system label Jan 15, 2025

davidgumberg mentioned this pull request Jan 15, 2025

ci: Bump centos stream 10 #31593

Merged

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 281b7fd to 8b3999a Compare January 16, 2025 18:52

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 8b3999a to 2b2c9e6 Compare January 16, 2025 19:00

DrahtBot added the CI failed label Jan 16, 2025

hebasto reviewed Jan 16, 2025

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

davidgumberg force-pushed the 1-15-24-configure_warnings branch 2 times, most recently from 09cd0c5 to 587f53e Compare January 17, 2025 02:05

hebasto reviewed Jan 17, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

fanquake mentioned this pull request Jan 17, 2025

cmake: Introduce WITH_PYTHON build option #31669

Closed

This was referenced Jan 17, 2025

cmake: Add CheckLinkerSupportsPIE module #31359

Merged

multiprocess: Add bitcoin wrapper executable #31375

Merged

Remove the legacy wallet and BDB dependency #28710

Merged

fanquake mentioned this pull request Jan 23, 2025

cmake: Fix -pthread flags presentation in summary #31724

Open

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 587f53e to 1a90741 Compare February 8, 2025 02:27

davidgumberg marked this pull request as ready for review February 8, 2025 02:47

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 1a90741 to fe58cfc Compare February 8, 2025 02:50

davidgumberg changed the title ~~build: Make config warnings fatal if -DWERROR=ON~~ build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON Feb 8, 2025

Sjors mentioned this pull request Mar 7, 2025

CI: Cmake warnings should be errors #31476

Open

This was referenced Mar 11, 2025

build: Use clang-cl to build on Windows natively #31507

Open

Require sqlite when building the wallet #31961

Merged

cmake, guix: Skip building tests in subtrees for releases #32054

Closed

DrahtBot added the Needs rebase label Mar 14, 2025

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 3e56d5b to df60131 Compare May 6, 2025 18:49

DrahtBot removed the Needs rebase label May 6, 2025

DrahtBot mentioned this pull request May 6, 2025

cmake: Check user-defined APPEND_*FLAGS variables early #32367

Open

DrahtBot added the Needs rebase label May 7, 2025

davidgumberg force-pushed the 1-15-24-configure_warnings branch from df60131 to f4a9073 Compare May 29, 2025 05:45

davidgumberg force-pushed the 1-15-24-configure_warnings branch from f4a9073 to 4b4e5d4 Compare May 29, 2025 05:47

DrahtBot removed the Needs rebase label May 29, 2025

DrahtBot mentioned this pull request Jun 5, 2025

ci: Rewrite test-each-commit as py script #32680

Merged

DrahtBot added the Needs rebase label Jun 11, 2025

davidgumberg added 3 commits June 10, 2025 17:20

build: WCONFIGURE_ERROR makes config errors fatal.

3d84ddb

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

ci: -DWCONFIGURE_ERROR=ON in CI jobs

f6cfb58

guix: -DWCONFIGURE_ERROR=ON for guix builds

b2c1f5b

davidgumberg force-pushed the 1-15-24-configure_warnings branch from 4b4e5d4 to b2c1f5b Compare June 11, 2025 00:20

DrahtBot removed the Needs rebase label Jun 11, 2025

maflcko reviewed Jul 3, 2025

View reviewed changes

.github/ci-test-each-commit-exec.sh Show resolved Hide resolved

maflcko added the DrahtBot Guix build requested label Jul 3, 2025

fanquake mentioned this pull request Jul 3, 2025

cmake: Use AUTHOR_WARNING for warnings #32865

Closed

davidgumberg closed this Jul 3, 2025

DrahtBot removed the DrahtBot Guix build requested label Jul 3, 2025

	find_package(Python3 3.10 COMPONENTS Interpreter)
	if(Python3_EXECUTABLE)
	set(PYTHON_COMMAND ${Python3_EXECUTABLE})
	else()
	list(APPEND configure_warnings
	"Minimum required Python not found. Utils and rpcauth tests are disabled."
	)
	endif()

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

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

Uh oh!

Conversation

davidgumberg commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

DrahtBot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented Jan 16, 2025

Uh oh!

fanquake commented Jan 16, 2025

Uh oh!

fanquake commented Jan 16, 2025

Uh oh!

davidgumberg commented Jan 16, 2025

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 17, 2025

Uh oh!

fanquake commented Jan 17, 2025

Uh oh!

luke-jr commented Jan 23, 2025

Uh oh!

hebasto commented Jan 23, 2025

Uh oh!

maflcko commented Jan 23, 2025

Uh oh!

davidgumberg commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidgumberg commented Feb 19, 2025

Uh oh!

davidgumberg commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidgumberg commented May 29, 2025

Uh oh!

Uh oh!

fanquake commented Jul 3, 2025

Uh oh!

davidgumberg commented Jul 3, 2025

Uh oh!

DrahtBot commented Jul 3, 2025

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

Uh oh!

Uh oh!

davidgumberg commented Jan 15, 2025 •

edited

Loading

DrahtBot commented Jan 15, 2025 •

edited

Loading

fanquake commented Jan 17, 2025 •

edited

Loading

davidgumberg commented Feb 8, 2025 •

edited

Loading

davidgumberg commented May 6, 2025 •

edited

Loading