cmake: Use `AUTHOR_WARNING` for warnings

fanquake commented at 11:21 am on July 3, 2025: member

Rather than create our own warning logging/handling, which isn’t ideal (#31476), use CMakes AUTHOR_WARNING (also pointed out by purpleKarrot here: #31665 (comment)).

Would result in failures like:

0-- Performing Test LINKER_SUPPORTS__WL__Z_SEPARATE_CODE - Success
1-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Required is at least version "3.10")
2CMake Error (dev) at CMakeLists.txt:592 (message):
3  Minimum required Python not found.  Rpcauth tests are disabled.
4This error is for project developers. Use -Wno-error=dev to suppress it.
5
6-- Configuring incomplete, errors occurred!

Alternative to #31665 & #31669. Would close #31476.

DrahtBot added the label Build system on Jul 3, 2025

DrahtBot commented at 11:21 am on July 3, 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/32865.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Stale ACK	purpleKarrot, ryanofsky

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:

#33144 (build: Set AUTHOR_WARNING on warnings by maflcko)

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.

in ci/test/03_test_script.sh:121 in d80fb7a752 outdated

117@@ -118,7 +118,7 @@ BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST}
118 mkdir -p "${BASE_BUILD_DIR}"
119 cd "${BASE_BUILD_DIR}"
120 
121-BITCOIN_CONFIG_ALL="$BITCOIN_CONFIG_ALL -DCMAKE_INSTALL_PREFIX=$BASE_OUTDIR"
122+BITCOIN_CONFIG_ALL="$BITCOIN_CONFIG_ALL -DCMAKE_INSTALL_PREFIX=$BASE_OUTDIR -Werror=dev"

maflcko commented at 11:41 am on July 3, 2025:

should this also be enabled in .github/ci-test-each-commit-exec.py, .github/workflows/ci.yml, and contrib/guix/libexec/build.sh? Otherwise, it is possible that warnings are missed in those contexts?

fanquake commented at 12:46 pm on July 3, 2025:

I’ve added .github/ci-test-each-commit-exec.py & .github/workflows/ci.yml. I’ll add a commit for Guix once the build passes.

fanquake commented at 1:22 pm on July 3, 2025:

Added a commit for Guix as well.

fanquake commented at 11:46 am on July 3, 2025: member

cc @purpleKarrot

in CMakeLists.txt:711 in a167325325 outdated

704-    message("  ******\n")
705-    foreach(warning IN LISTS configure_warnings)
706-      message(WARNING "${warning}")
707-    endforeach()
708-    message("  ******\n")
709-endif()

hebasto commented at 11:56 am on July 3, 2025:

All warnings were intentionally printed just after the summary to draw the user’s attention to them.

maflcko commented at 9:02 am on July 4, 2025:

Yeah, seems fine to keep this at the end for users and devs that have it not set? Maybe just use a trivial one-line patch?

 0diff --git a/CMakeLists.txt b/CMakeLists.txt
 1index 4da12e0f46..c3dea019b4 100644
 2--- a/CMakeLists.txt
 3+++ b/CMakeLists.txt
 4@@ -710,6 +710,7 @@ if(configure_warnings)
 5       message(WARNING "${warning}")
 6     endforeach()
 7     message("  ******\n")
 8+    message(AUTHOR_WARNING "Warnings have been encountered!")
 9 endif()
10 
11 # We want all build properties to be encapsulated properly.

Also, could enable Werror in "name": "dev-mode", preset?

fanquake commented at 12:57 pm on July 4, 2025:

Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (configure_warnings doesn’t come from CMake). The behavior of master is that the only warnings that could appear here are no python, tried to turn off ccache but couldn't, PIE doesn't work, (and no other CMake warnings/output).

Also, could enable Werror in “name”: “dev-mode”, preset?

Will add.

maflcko commented at 9:06 am on July 9, 2025:

Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (configure_warnings

Yes, that was my suggestion: Replace the commit with a trivial one-line patch.

hebasto commented at 11:10 am on July 10, 2025:

Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (configure_warnings

Yes, that was my suggestion: Replace the commit with a trivial one-line patch.

I support @maflcko’s suggestion.

maflcko commented at 2:29 pm on August 6, 2025:

Done in #33144. It should be trivial to revert that one-line patch and rebase this pull on top of it, if there is need.

fanquake force-pushed on Jul 3, 2025

DrahtBot added the label CI failed on Jul 3, 2025

DrahtBot commented at 12:45 pm on July 3, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross, gui, no tests: https://github.com/bitcoin/bitcoin/runs/45287145804 LLM reason (✨ experimental): The CI failure is caused by an linker error: “ld64.lld: error: unknown argument ‘–exclude-libs’” indicating that the linker does not support the ‘–exclude-libs’ option used in the build process.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake force-pushed on Jul 3, 2025

fanquake marked this as ready for review on Jul 3, 2025

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

fanquake commented at 2:58 pm on July 3, 2025: member

Guix Build:

 07c9bae09c11b66251bcab7abe66d3446da64ca2386fbc67066c1f8194df778b9  guix-build-04c4cc059e44/output/aarch64-linux-gnu/SHA256SUMS.part
 1ffb0670da219af40ce2c986e33b8446bbc6d60ebfa31fd142f91a22a50a3ba89  guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu-debug.tar.gz
 21b54fb59042b8abc7d2b7079de3f9b16770f6434c41049aebf4e0bbb4278596b  guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu.tar.gz
 3fc14d2a51e6a1b12e8a7b96450e15809bf1db5dd1a8c5a378763641e20f871a4  guix-build-04c4cc059e44/output/arm-linux-gnueabihf/SHA256SUMS.part
 4bb0fe708f4fb50a917c29299de96d7afb7d7e387ab55dfc7f75d162afaa9e493  guix-build-04c4cc059e44/output/arm-linux-gnueabihf/bitcoin-04c4cc059e44-arm-linux-gnueabihf-debug.tar.gz
 5d0691b820933c849e5e52f3b2deca15b43866c7260fad403cfa396f224b5bf49  guix-build-04c4cc059e44/output/arm-linux-gnueabihf/bitcoin-04c4cc059e44-arm-linux-gnueabihf.tar.gz
 60de94186e11f0e483a82bf3ce073eda190e6a0a0c84935299fa331c2fe62c9e5  guix-build-04c4cc059e44/output/arm64-apple-darwin/SHA256SUMS.part
 78f2068572bd83964591d6d420c72f4ac9388aecd152e75e9319ae35238f83fc7  guix-build-04c4cc059e44/output/arm64-apple-darwin/bitcoin-04c4cc059e44-arm64-apple-darwin-codesigning.tar.gz
 89e31de4e53573c2479fc1061dd0769861d186de30c52dc8a27e1cd5aa0519d07  guix-build-04c4cc059e44/output/arm64-apple-darwin/bitcoin-04c4cc059e44-arm64-apple-darwin-unsigned.tar.gz
 9bb493e8cdb9939e595f0d9de9c0df446e71f3bf9298577b70442e6440d65be00  guix-build-04c4cc059e44/output/arm64-apple-darwin/bitcoin-04c4cc059e44-arm64-apple-darwin-unsigned.zip
10b764580cf796e3350179c054598228e350bb767e3f21654b8541474c226bb201  guix-build-04c4cc059e44/output/dist-archive/bitcoin-04c4cc059e44.tar.gz
11f741c4d7b5d72241624216bd07ae8454f02decac70129cd92a72d9016de82ca1  guix-build-04c4cc059e44/output/powerpc64-linux-gnu/SHA256SUMS.part
12fd2f76976f1cf2ebf995d0b24f7545d6a863673e3629940967617d3cae6da752  guix-build-04c4cc059e44/output/powerpc64-linux-gnu/bitcoin-04c4cc059e44-powerpc64-linux-gnu-debug.tar.gz
13ecf9fcfaf75c5542bd2b3346efebc01299bdeeab9604916a8dc1174993216cfc  guix-build-04c4cc059e44/output/powerpc64-linux-gnu/bitcoin-04c4cc059e44-powerpc64-linux-gnu.tar.gz
14ecb8cdb345525219203d4921ba3586315649d3b9c46417ebe8bc1ebb40aff52f  guix-build-04c4cc059e44/output/riscv64-linux-gnu/SHA256SUMS.part
15d5cb27d211f7d5e639708b057e4cf3bb738d7c8b0b13c9023a6bee3a1af598c3  guix-build-04c4cc059e44/output/riscv64-linux-gnu/bitcoin-04c4cc059e44-riscv64-linux-gnu-debug.tar.gz
161b807b6e2fe23d63e9d84b1d27c4ec0bfae45f8595bb20ae015dbdf62b45462e  guix-build-04c4cc059e44/output/riscv64-linux-gnu/bitcoin-04c4cc059e44-riscv64-linux-gnu.tar.gz
17ec7c9113b4acad38b112812ce9bc0bf6dcffd9e6eb9b6f89bbf08da42cb93ae1  guix-build-04c4cc059e44/output/x86_64-apple-darwin/SHA256SUMS.part
1882bc0df4fcefe8ecd6dc9336ab17de24e4d42812de0e860f34c7b1ebe2fd8552  guix-build-04c4cc059e44/output/x86_64-apple-darwin/bitcoin-04c4cc059e44-x86_64-apple-darwin-codesigning.tar.gz
1905320e706a0082b7cdd63aa8d004046aeb042f5742bae2c1531a38c0885d3f19  guix-build-04c4cc059e44/output/x86_64-apple-darwin/bitcoin-04c4cc059e44-x86_64-apple-darwin-unsigned.tar.gz
20047df0d762d3f67ed814cc98a375781d3e4588d6fb0ae226b73278ef9339d9b9  guix-build-04c4cc059e44/output/x86_64-apple-darwin/bitcoin-04c4cc059e44-x86_64-apple-darwin-unsigned.zip
21ef85f647d71c0d5999acf9c2ab84a13b2971475d64781d7d595122e129dc8565  guix-build-04c4cc059e44/output/x86_64-linux-gnu/SHA256SUMS.part
22913e6c99c854298d4414441dfee8bb0e0cb378a6c5e7470080c2f68da24152bf  guix-build-04c4cc059e44/output/x86_64-linux-gnu/bitcoin-04c4cc059e44-x86_64-linux-gnu-debug.tar.gz
23a5c01a598b1dff050ea820416266989207f62db9937e4836025def585bb9ae5f  guix-build-04c4cc059e44/output/x86_64-linux-gnu/bitcoin-04c4cc059e44-x86_64-linux-gnu.tar.gz
2443f67909195c5b32f337659489758ec350eb555d31c33c952b58478e942a22d1  guix-build-04c4cc059e44/output/x86_64-w64-mingw32/SHA256SUMS.part
252f09e27676290afa2bb0172a6ce94a5e0188cd516f3771cb9a85ae9102cad2ef  guix-build-04c4cc059e44/output/x86_64-w64-mingw32/bitcoin-04c4cc059e44-win64-codesigning.tar.gz
265938f7b23de5701fa02720290b008c1b4c7f5ca95faa45051ab1dfb6c9c32256  guix-build-04c4cc059e44/output/x86_64-w64-mingw32/bitcoin-04c4cc059e44-win64-debug.zip
277d2008ba3affffb705b9271c0e29d88ccb1e3c1dd225ffc91963da2ecbc49fb2  guix-build-04c4cc059e44/output/x86_64-w64-mingw32/bitcoin-04c4cc059e44-win64-setup-unsigned.exe
288aa293975b097d2890fe9efb521a9a330d9f3332e74a4bec63a9102c2ecbf799  guix-build-04c4cc059e44/output/x86_64-w64-mingw32/bitcoin-04c4cc059e44-win64-unsigned.zip

purpleKarrot commented at 4:01 pm on July 3, 2025: contributor

ACK 04c4cc059e44a6d1b4f520af10c649648a002ebc

DrahtBot removed the label CI failed on Jul 3, 2025

DrahtBot commented at 8:10 am on July 4, 2025: contributor

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

File	commit 51ccc71b1bf9ab3cd211f48b9805d3b7b957ced8(master)	commit 45a458fd01bb32a16954f825325d5e7135d46732(pull/32865/merge)
*-aarch64-linux-gnu-debug.tar.gz	`b4f62a03fed68e6e...`	`17d5966123ada5cb...`
*-aarch64-linux-gnu.tar.gz	`67da2dd39446e055...`	`f7453b63f1556576...`
*-arm-linux-gnueabihf-debug.tar.gz	`224d178ed09b58ce...`	`10529d1e616e0215...`
*-arm-linux-gnueabihf.tar.gz	`2d8d8e1901fa7f17...`	`09039650386fa94a...`
*-arm64-apple-darwin-codesigning.tar.gz	`aaa96ec17bc009bf...`	`3838c6763cfb8a4b...`
*-arm64-apple-darwin-unsigned.tar.gz	`6351e4ec85221874...`	`13b314f13ed66030...`
*-arm64-apple-darwin-unsigned.zip	`5b5977fab0619831...`	`f815b8dcfd4446cc...`
*-powerpc64-linux-gnu-debug.tar.gz	`4a6d90213362c408...`	`7ce7745554f02d03...`
*-powerpc64-linux-gnu.tar.gz	`623b924620830e3f...`	`513e36358d096f3e...`
*-riscv64-linux-gnu-debug.tar.gz	`29acf8d38a47618f...`	`9cdd17e576c70584...`
*-riscv64-linux-gnu.tar.gz	`396297bf42899816...`	`56ea7076196578cf...`
*-x86_64-apple-darwin-codesigning.tar.gz	`3eaa5212fcca90db...`	`2a930ac29ca4a02b...`
*-x86_64-apple-darwin-unsigned.tar.gz	`2baee23a3e8aa15a...`	`e313a52242151801...`
*-x86_64-apple-darwin-unsigned.zip	`99e52233a0e9db27...`	`3b561705e62916ea...`
*-x86_64-linux-gnu-debug.tar.gz	`4246718f7c02ecdf...`	`327449434c43b21a...`
*-x86_64-linux-gnu.tar.gz	`57491003214db776...`	`83e913b03cd534c1...`
*.tar.gz	`d014f5b396d4a0d4...`	`82ccd3a5878b3a0c...`
SHA256SUMS.part	`5e57caac3e97ba45...`	`b47d852635150b67...`
guix_build.log	`c32f66181eb0e72c...`	`977b2aeb3198d668...`
guix_build.log.diff		`75d6680c4b3970c4...`

DrahtBot removed the label DrahtBot Guix build requested on Jul 4, 2025

fanquake force-pushed on Jul 4, 2025

luke-jr commented at 8:35 pm on July 7, 2025: member

Concept NACK: These warnings don’t appear to be just for developers, but also relevant to builders.

fanquake commented at 8:50 pm on July 7, 2025: member

Concept NACK: These warnings don’t appear to be just for developers, but also relevant to builders.

Not sure I understand your NACK. The warnings will still be shown to builders; using AUTHOR_WARNING just gives us a proper mechanism to turn warnings into errors.

ryanofsky approved

ryanofsky commented at 1:08 pm on July 8, 2025: contributor

Code review ACK bf8bf7408c30b54d953c2b983a66c693e5eb6f21.

IIUC, the last 3 commits passing -Werror=dev to cmake in CI, guix, and developer presents seem like clearly good changes, because they turn cmake internal warnings and message(AUTHOR_WARNING "...") statements into errors which stop the build so they can’t be ignored.

The other commit 02955f3af3bcdc9d978784cad6a98badf5d1117b is changing our current message(WARNING "...") statements into message(AUTHOR_WARNING "...") statements so these warnings will also become errors when -Werror=dev is used. This change seems a little kludgy because author warnings seem meant for people maintaining the build system, not just using it, but all of the warnings that are changed seem to be about ignored options provided by users. However, cmake doesn’t provide an easy way to treat normal warnings as errors, so using AUTHOR_WARNING instead of WARNING seems helpful even if it doesn’t make sense semantically.

ryanofsky commented at 1:16 pm on July 8, 2025: contributor

Not sure I understand your NACK. The warnings will still be shown to builders; using AUTHOR_WARNING just gives us a proper mechanism to turn warnings into errors.

I think luke’s concern might be that author warnings can be suppressed with -Wno-dev and might be suppressed by default in certain environments, since author warnings are only supposed to be relevant to project maintainers, not to users.

In 02955f3af3bcdc9d978784cad6a98badf5d1117b we are turning normal warnings about ignored options into the author warnings so the change does not really make sense semantically and could cause these warnings to be ignored or hidden in cases where they might not be ignored before.

But since cmake only provides an easy way to turn author warnings into errors, not other types of warnings, I do think this change is probably good overall, just maybe not ideal.

fanquake commented at 9:01 am on July 9, 2025: member

@ryanofsky Yea, it’s a shame CMake doesn’t have something builtin to do this. I think this change, while not perfect, is better than the current state of having our own warning handling (where warnings are still easily missed (#31476), we have no option to turn them into errors, and someone still needs to pass -Wdev to get CMake warnings turned into errors). As well as the alternatives of, just adding even more options to the build system (#31669) which doesn’t actually fix the issue generically, and still requires someone to use the CMake options if they want all errors, or adding even more custom warning handling (#31665).

purpleKarrot commented at 9:16 am on July 9, 2025: contributor

@fanquake, what exactly would fix the issue upstream? A generic -Werror option (without the = part) that simply turns all warnings into errors?

fanquake commented at 9:22 am on July 9, 2025: member

A generic -Werror option (without the = part) that simply turns all warnings into errors?

I think so. The downside in this PR is that we have to somewhat (even if semantically) misuse AUTHOR_WARNING(), to get the warning into error behaviour. Ideally we could just use WARNING(), and -Werror would turn those warnings into errors.

fanquake force-pushed on Jul 10, 2025

DrahtBot added the label CI failed on Jul 10, 2025

DrahtBot removed the label CI failed on Jul 10, 2025

hebasto referenced this in commit 6a13a6106e on Jul 11, 2025

cmake: use AUTHOR_WARNING

Co-authored-by: Daniel Pfeifer <daniel@pfeifer-mail.de>

bbaf3ecef4

fanquake force-pushed on Jul 14, 2025

cmake: Use AUTHOR_WARNING for warnings #32865

Code Coverage & Benchmarks

Reviews

Conflicts

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

cmake: Use `AUTHOR_WARNING` for warnings #32865