build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro #1241

1. 
   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:

   0cc1: 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.

2. 
   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?
3. 
   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:

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

4. hebasto force-pushed on Mar 13, 2023
5. 
   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?

6. 
   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.

7. 
   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?

8. 
   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.”

9. build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro

   Co-authored-by: Tim Ruffing <crypto@timruffing.de>

   3addb4c1e8
10. hebasto force-pushed on Mar 26, 2023
11. real-or-random approved
12. 
    real-or-random commented at 9:41 am on March 26, 2023: contributor

    utACK 3addb4c1e8a50df7dcf4465a7f149f78bf5af78b
13. 
    jonasnick commented at 6:59 pm on March 28, 2023: contributor

    ACK 3addb4c1e8a50df7dcf4465a7f149f78bf5af78b
14. jonasnick merged this on Mar 28, 2023
15. jonasnick closed this on Mar 28, 2023

    ---

16. hebasto deleted the branch on Mar 28, 2023
17. sipa referenced this in commit e1552d578e on Apr 11, 2023
18. sipa referenced this in commit c981671e9b on Apr 14, 2023
19. 
    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.
20. real-or-random cross-referenced this on Apr 18, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by real-or-random
21. 
    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..

22. real-or-random cross-referenced this on Apr 18, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto
23. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
24. RandyMcMillan referenced this in commit 3cc75121b3 on May 27, 2023
25. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
26. vmta referenced this in commit 8f03457eed on Jul 1, 2023
27. alokeutpal approved
28. jonasnick cross-referenced this on Jul 21, 2023 from issue Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 by jonasnick
29. jonasnick cross-referenced this on Jul 21, 2023 from issue Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 by jonasnick

Contributors
hebasto fanquake real-or-random jonasnick theuni alokeutpal