Skip to content

build: Set AUTHOR_WARNING on warnings #33144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
maflcko wants to merge 1 commit into
base: master
Choose a base branch
from
Open

build: Set AUTHOR_WARNING on warnings #33144

maflcko wants to merge 1 commit into from
+1 −0

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 6, 2025 •
edited
Loading

Now that the cmake setting -Werror=dev is set since commit 6a13a61 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:

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

Fixes #31476.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 6, 2025 •
edited
Loading

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.

@maflcko maflcko mentioned this pull request Aug 6, 2025
Closed
@l0rinc
Copy link
Contributor

l0rinc commented Aug 6, 2025 •
edited
Loading

Rebased, tested it with:

diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt	(revision 27e4b8c6856194fa8db028b6f7356f83ea3d7e3a)
+++ b/CMakeLists.txt	(date 1754504896922)
@@ -186,6 +186,7 @@
 string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")

 set(configure_warnings)
+list(APPEND configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144")

 include(CheckLinkerSupportsPIE)
 check_linker_supports_pie(configure_warnings)

running:

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

Prints before the change:

CMake Warning at CMakeLists.txt:709 (message):
  Testing https://github.com/bitcoin/bitcoin/pull/33144

After the change:

CMake Warning at CMakeLists.txt:709 (message):
  Testing https://github.com/bitcoin/bitcoin/pull/33144
--

CMake Warning (dev) at CMakeLists.txt:712 (message):
  Warnings have been encountered!
This warning is for project developers.  Use -Wno-dev to suppress it.

ACK fa6497b

@l0rinc l0rinc mentioned this pull request Aug 7, 2025
Closed
@maflcko maflcko requested a review from hebasto August 20, 2025 19:50
stickies-v
stickies-v approved these changes Sep 1, 2025
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa6497b

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:

  1. 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
  2. missing PIE support
  3. ccache misconfiguration

For:

  1. I think a FATAL_ERROR when functional tests are configured (e.g. f529c14) might be appropriate.
  2. it similarly might make sense to trigger a FATAL_ERROR (e.g. c988ca5), I'm not sure if we actually want to support compiling without PIE?
  3. 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 7cc9a08...stickies-v:2025-09/cmake-remove-configure-warnings just to try it out.

@maflcko
Copy link
Member Author

maflcko commented Sep 2, 2025

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 stickies-v mentioned this pull request Sep 2, 2025
Closed
@stickies-v
Copy link
Contributor

stickies-v commented Sep 2, 2025

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
Copy link
Contributor

davidgumberg commented Sep 4, 2025 •
edited
Loading

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 7cc9a08...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, 7cc9a08...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:

bitcoin/build-aux/m4/bitcoin_qt.m4

Lines 5 to 17 in 44d8b13

dnl Helper for cases where a qt dependency is not met.
dnl Output: If qt version is auto, set bitcoin_enable_qt to false. Else, exit.
AC_DEFUN([BITCOIN_QT_FAIL],[
if test "$bitcoin_qt_want_version" = "auto" && test "$bitcoin_qt_force" != "yes"; then
if test "$bitcoin_enable_qt" != "no"; then
AC_MSG_WARN([$1; bitcoin-qt frontend will not be built])
fi
bitcoin_enable_qt=no
bitcoin_enable_qt_test=no
else
AC_MSG_ERROR([$1])
fi
])

Edit: Edit:

Also see discussion here: #33290 (comment)

@maflcko
Copy link
Member Author

maflcko commented Sep 7, 2025

+GHA CI

@maflcko maflcko closed this Sep 7, 2025
@maflcko maflcko reopened this Sep 7, 2025
@hebasto
Copy link
Member

hebasto commented Nov 23, 2025

cc @purpleKarrot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hebasto hebasto Awaiting requested review from hebasto

1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI: Cmake warnings should be errors

6 participants

@maflcko @DrahtBot @l0rinc @stickies-v @davidgumberg @hebasto