build: Set AUTHOR_WARNING on warnings

maflcko commented at 2:27 pm on August 6, 2025: member

Now that the cmake setting -Werror=dev is set since commit 6a13a6106e3c1ebe95ba6430184d6260a7b942bd for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.

So do that with a single AUTHOR_WARNING.

This can be tested by introducing a bug, like:

 0diff --git a/CMakeLists.txt b/CMakeLists.txt
 1index 6017775fa7..5610e03c66 100644
 2--- a/CMakeLists.txt
 3+++ b/CMakeLists.txt
 4@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE STRING "")
 5 # improves compatibility with Python version managers that use shims.
 6 set(Python3_FIND_UNVERSIONED_NAMES FIRST CACHE STRING "")
 7 mark_as_advanced(Python3_FIND_FRAMEWORK Python3_FIND_UNVERSIONED_NAMES)
 8-find_package(Python3 3.10 COMPONENTS Interpreter)
 9+find_package(Python3 3.210 COMPONENTS Interpreter)
10 if(NOT TARGET Python3::Interpreter)
11   list(APPEND configure_warnings
12     "Minimum required Python not found."

Fixes #31476.

build: Set AUTHOR_WARNING on warnings fa6497ba71

DrahtBot added the label Build system on Aug 6, 2025

DrahtBot commented at 2:28 pm on August 6, 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/33144.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

l0rinc commented at 6:45 pm on August 6, 2025: contributor

Rebased, tested it with:

 0diff --git a/CMakeLists.txt b/CMakeLists.txt
 1--- a/CMakeLists.txt	(revision 27e4b8c6856194fa8db028b6f7356f83ea3d7e3a)
 2+++ b/CMakeLists.txt	(date 1754504896922)
 3@@ -186,6 +186,7 @@
 4 string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")
 5
 6 set(configure_warnings)
 7+list(APPEND configure_warnings "Testing [#33144](/bitcoin-bitcoin/33144/)")
 8
 9 include(CheckLinkerSupportsPIE)
10 check_linker_supports_pie(configure_warnings)

running:

0rm -rfd build && cmake -B build 2>&1 | grep -C1 Warning

Prints before the change:

0CMake Warning at CMakeLists.txt:709 (message):
1  Testing [#33144](/bitcoin-bitcoin/33144/)

After the change:

0CMake Warning at CMakeLists.txt:709 (message):
1  Testing [#33144](/bitcoin-bitcoin/33144/)
2--
3
4CMake Warning (dev) at CMakeLists.txt:712 (message):
5  Warnings have been encountered!
6This warning is for project developers.  Use -Wno-dev to suppress it.

ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74

maflcko requested review from hebasto on Aug 20, 2025

stickies-v approved

stickies-v commented at 8:17 pm on September 1, 2025: contributor

ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74

It makes sense to have an option to have configuration warnings dealt as errors, and it’s unfortunate that the most elegant way to do that is by somewhat abusing AUTHOR_WARNING, but this seems like the least bad solution to me, until CMake has a -Werror=all option.

I’ve also been thinking about an alternative approach. configure_warnings is currently used for:

missing Python interpreter, which is necessary (without build failure) for the functional test suite, and is necessary (but build will fail) for maintenance and macos deploy targets
missing PIE support
ccache misconfiguration

For:

I think a FATAL_ERROR when functional tests are configured (e.g. f529c145aa0c418618932bf2d730006c7630166f) might be appropriate.
it similarly might make sense to trigger a FATAL_ERROR (e.g. c988ca5d41105cea7c72a3ad6ab03c87f2124f26), I’m not sure if we actually want to support compiling without PIE?
this might be more controversial, but I don’t think WITH_CCACHE should be part of our build system.

Those changes combined would remove configure_warnings entirely, removing the need for this PR, but it obviously is a much bigger change. I’ve implemented it in https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings just to try it out.

maflcko commented at 7:22 am on September 2, 2025: member

Sounds good. Happy to put this in draft for now to see if your removal of configure_warnings makes it in, but I haven’t looked at 2. and 3., if they make sense.

stickies-v commented at 11:55 am on September 2, 2025: contributor

Sounds good.

Thanks for the feedback, I’ve opened #33278.

Happy to put this in draft for now

The changes in this/your PR are small, straightforward, can easily be reverted, and offer immediate practical benefit, so I think it makes sense to not block this on my much less straightforward alternative, so my ACK still stands and I’d be happy if this makes progress.

davidgumberg commented at 10:07 pm on September 4, 2025: contributor

I don’t see how abusing the semantics of AUTHOR_WARNING is preferable to adding our own flag (#31665), but I prefer fixing #31476, so not a big deal.

I think the changes implemented by @stickies-v in https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings make sense individually, but the sum total of this approach is that it shifts the experience of building bitcoin core from cmake -B build doing what it can to configure a reasonable build for you given what it can find to: attempt build, read errors to see what it’s mad about, install a dependency or disable a feature, repeat, and that is a much worse experience in my opinion.

Trying our best to configure a build and printing warnings and letting mindful users read and address them, and fixing #31476 so that warnings are never tolerated in CI / dev preset builds seems like a better approach to me.

Edit: On further thought, https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings wouldn’t really be a shift from the current approach, we don’t generally change build configuration based on what is found on the user’s system, that was true in the old build system:

https://github.com/bitcoin/bitcoin/blob/44d8b13c81e5276eb610c99f227a4d090cc532f6/build-aux/m4/bitcoin_qt.m4#L5-L17

Edit: Edit:

Also see discussion here: #33290 (comment)

maflcko commented at 11:20 am on September 7, 2025: member

+GHA CI

maflcko closed this on Sep 7, 2025

maflcko reopened this on Sep 7, 2025

build: Set AUTHOR_WARNING on warnings #33144

Code Coverage & Benchmarks

Reviews

Conflicts