build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro #1241

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:230313-werror changing 1 files +7 −4

hebasto commented at 4:42 PM on March 13, 2023: member
While the SECP_TRY_APPEND_DEFAULT_CFLAGS macro works with the current compiler flag set, it is not perfect.

For example, it will accept -fvisibility-inlines-hidden flag, which actually causes:
```
cc1: warning: command-line option ‘-fvisibility-inlines-hidden’ is valid for C++/ObjC++ but not for C
```
This PR improves robustness of the SECP_TRY_APPEND_DEFAULT_CFLAGS macro.

FWIW, the same approach is used in our CMake-based build system, and in Bitcoin Core.
fanquake commented at 5:49 PM on March 13, 2023: member

Looks like the comment needs updating as well? What happens after this change, with (supported versions of) Clang?

hebasto commented at 8:12 PM on March 13, 2023: member

What happens after this change, with (supported versions of) Clang?

I've tested clang-8 and clang-14. The only diff in configure script output looks like that:

--- /home/hebasto/config-master
+++ /home/hebasto/config-pr1241
@@ -74,7 +74,7 @@
 checking if libtool supports shared libraries... yes
 checking whether to build shared libraries... yes
 checking whether to build static libraries... yes
-checking if clang-14 supports -Werror=unknown-warning-option... yes
+checking if clang-14 supports -Werror... yes
 checking if clang-14 supports -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef... yes
 checking if clang-14 supports -Wno-overlength-strings... yes
 checking if clang-14 supports -Wall... yes

hebasto force-pushed on Mar 13, 2023
hebasto commented at 8:18 PM on March 13, 2023: member

Updated cac8fa4cd52c6086949a84cd6916ec15f111214a -> 686fa5d9ebbafdfc8f7fe41fdbbb0f8e46d96d65 (pr1241.01 -> pr1241.02, diff).

Addressed @fanquake's comment:

Looks like the comment needs updating as well?
real-or-random commented at 2:18 PM on March 17, 2023: contributor

Concept ACK

I think the comment could also point out that -Werror may in principle be supported but warnings are already present ( https://github.com/bitcoin/bitcoin/blob/f088949fcfe6ba5000caa2f6adc6803e81925afb/configure.ac#L344-L347). That's rather subtle.
hebasto commented at 11:02 AM on March 20, 2023: member

@real-or-random

I think the comment could also point out that -Werror may in principle be supported but warnings are already present

Mind suggesting a wording for the comment, please?
real-or-random commented at 8:06 AM on March 26, 2023: contributor

Mind suggesting a wording for the comment, please?

"Note that failure to append -Werror does not necessarily mean that -Werror is not supported. The compiler may already be warning about something unrelated, for example about some path issue. If that is the case, -Werror cannot be used because all of those warnings would be turned into errors."
build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
3addb4c1e8
hebasto force-pushed on Mar 26, 2023
real-or-random approved
real-or-random commented at 9:41 AM on March 26, 2023: contributor

utACK 3addb4c1e8a50df7dcf4465a7f149f78bf5af78b
jonasnick commented at 6:59 PM on March 28, 2023: contributor

ACK 3addb4c1e8a50df7dcf4465a7f149f78bf5af78b
jonasnick merged this on Mar 28, 2023
jonasnick closed this on Mar 28, 2023
hebasto deleted the branch on Mar 28, 2023
sipa referenced this in commit e1552d578e on Apr 11, 2023
sipa referenced this in commit c981671e9b on Apr 14, 2023
theuni commented at 5:40 PM on April 14, 2023: contributor

Following up on this, I didn't realize this existed: CMAKE_COMPILE_WARNING_AS_ERROR. It'd be nice to opt-in to that behavior for v3.24+, or at least make note of it.
real-or-random commented at 10:58 AM on April 18, 2023: contributor

Following up on this, I didn't realize this existed: CMAKE_COMPILE_WARNING_AS_ERROR. It'd be nice to opt-in to that behavior for v3.24+, or at least make note of it.

Added to the list in #1235..
hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
RandyMcMillan referenced this in commit 3cc75121b3 on May 27, 2023
vmta referenced this in commit e1120c94a1 on Jun 4, 2023
vmta referenced this in commit 8f03457eed on Jul 1, 2023
delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
janus referenced this in commit 05145db5ff on Sep 3, 2023
backpacker69 referenced this in commit e66b1bd882 on Mar 5, 2024
div72 referenced this in commit af627d47c3 on Apr 12, 2025
str4d referenced this in commit f634fc6004 on Jun 4, 2025
Fabcien referenced this in commit 55876b300d on Apr 17, 2026
Fabcien referenced this in commit 079d24940b on Apr 17, 2026

Contributors

Linked (view graph)

#1235 build: Meta-issue for follow-ups to initial CMake merge (#1113)