cmake: Inherit WERROR setting for secp256k1 build #33297

1. 
   jmoik commented at 6:34 pm on September 3, 2025: none

   This change prevents unexpected CLI output when compiling with -DWERROR=ON (treat warnings as errors). This setting is now properly propagated to the secp256k1 subproject build. Previously, secp256k1 would be built without the -Werror flag even when the main project was configured to treat warnings as errors, creating inconsistent build behavior. https://github.com/bitcoin/bitcoin/issues/33284
2. DrahtBot added the label Build system on Sep 3, 2025
3. 
   DrahtBot commented at 6:34 pm on September 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/33297.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	purpleKarrot
   Concept ACK	luke-jr, kevkevinpal

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

4. 
   purpleKarrot commented at 9:25 am on September 4, 2025: contributor

   I am going to NACK every PR that adds even more -Werror. 30f642952cb5bf39479bdbe467b3950f0d09324a was a mistake and should be reverted. The correct approach to this is CMAKE_COMPILE_WARNING_AS_ERROR.

   Yes, that requires CMake 3.24 and we allow 3.22 as the minimum. It does not matter. Just accept that the setting will have no effect on Ubuntu 22.04. :man_shrugging:

   cc @hebasto

5. 
   hebasto commented at 11:27 am on September 4, 2025: member

   30f6429 was a mistake…

   Here is some historical context.

   When migrating the build system from Autotools to CMake, there was a unanimous expectation that, at least initially, we should aim for 100% build option compatibility. That commit introduced CMake’s counterpart to Autotools’ --enable-werror:https://github.com/bitcoin/bitcoin/blob/6e62b705328f4a52875a1dd3bfb63fbba8586a01/configure.ac#L284-L289

6. 
   hebasto commented at 11:34 am on September 4, 2025: member

   Yes, that requires CMake 3.24 and we allow 3.22 as the minimum. It does not matter. Just accept that the setting will have no effect on Ubuntu 22.04. 🤷‍♂️

   That seems acceptable to me, as none of the CI jobs use CMake older that 3.24. And when someone sets CMAKE_COMPILE_WARNING_AS_ERROR with an older CMake, it issues a warning:

   0CMake Warning:
   1  Manually-specified variables were not used by the project:
   2
   3    CMAKE_COMPILE_WARNING_AS_ERROR
   

7. 
   purpleKarrot commented at 2:05 pm on September 4, 2025: contributor

   Here is some historical context.

   Don’t take it personal. You did a great job migrating from autotools to CMake. I have zero doubts about that.

8. 
   luke-jr commented at 1:01 am on September 18, 2025: member

   Concept ACK. As long as -DWERROR is supported, it should work correctly.

   Discussion about dropping it in favour of other solutions should be considered separately (and existing branches should get fixed even if it’s dropped in newer versions)

9. cmake: Inherit WERROR setting for secp256k1 build

   cmake: reuse working_compiler_werror_flag in secp256k1

   18d3d67016
10. in cmake/secp256k1.cmake:33 in ed1c975100

    24@@ -25,6 +25,15 @@ function(add_secp256k1 subdir)
    25   get_target_interface(SECP256K1_APPEND_CFLAGS "" sanitize_interface COMPILE_OPTIONS)
    26   string(STRIP "${SECP256K1_APPEND_CFLAGS} ${APPEND_CPPFLAGS}" SECP256K1_APPEND_CFLAGS)
    27   string(STRIP "${SECP256K1_APPEND_CFLAGS} ${APPEND_CFLAGS}" SECP256K1_APPEND_CFLAGS)
    28+  if(WERROR)
    29+    if(MSVC)
    30+      set(werror_flag "/WX")
    31+    else()
    32+      set(werror_flag "-Werror")
    33+    endif()
    

    ---

    ---

    

    hebasto commented at 7:32 am on September 18, 2025:

    Why not reuse the working_compiler_werror_flag variable?

    ---

    

    jmoik commented at 8:18 am on September 26, 2025:

    Good point, I was not aware of this variable, I have adjusted the code and tested it.

    ---

    

    maflcko commented at 8:21 am on September 26, 2025:

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
11. jmoik force-pushed on Sep 26, 2025
12. 
    kevkevinpal commented at 6:12 pm on September 27, 2025: contributor

    Concept ACK 18d3d67

    I agree with luke-jr that even if we decided to drop this in favor of another solution that should be considered separately

Contributors
jmoik DrahtBot purpleKarrot hebasto luke-jr maflcko kevkevinpal

Labels
Build system