cmake: Rename SECP256K1_LATE_CFLAGS and switch to Bitcoin Core’s approach #1546

1. 
   hebasto commented at 1:58 pm on June 21, 2024: member

   This PR address this https://github.com/hebasto/bitcoin/issues/239#issuecomment-2182713690:

   For consistency with libsecp256k1:

   Is this code block supposed to achieve the same as our SECP256K1_LATE_CFLAGS (implemented by a user-defined function all_targets_add_compile_options) in libsecp256k1?

   It is. But this approach guaranties to override even options that are abstracted by CMake, for instance #157 (comment).

   • If we agree that appending to rule variables is superior, should we also do this in libsecp256k1?

   • And/or should we rename the SECP256K1_LATE_CFLAGS variable to APPEND_CFLAGS?

2. cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`

   This change follows the naming convention currently used in the Bitcoin
   Core's CMake staging branch.

   c2764dbb99
3. 
   hebasto commented at 1:59 pm on June 21, 2024: member

   @real-or-random

   Your other comments from the discussion in https://github.com/hebasto/bitcoin/issues/239 have also been included.

4. in CMakeLists.txt:287 in 91febae005 outdated

   283@@ -284,14 +284,16 @@ if(SECP256K1_BUILD_BENCHMARK OR SECP256K1_BUILD_TESTS OR SECP256K1_BUILD_EXHAUST
   284   enable_testing()
   285 endif()
   286 
   287-set(SECP256K1_LATE_CFLAGS "" CACHE STRING "Compiler flags that are added to the command line after all other flags added by the build system.")
   288-include(AllTargetsCompileOptions)
   289+set(SECP256K1_APPEND_CFLAGS "" CACHE STRING "Compiler flags that are appended to the command line after all other flags added by the build system. It is intended for debugging and special builds.")
   

   ---

   ---

   

   real-or-random commented at 3:09 pm on June 21, 2024:

   nit:

   0set(SECP256K1_APPEND_CFLAGS "" CACHE STRING "Compiler flags that are appended to the command line after all other flags added by the build system. This variable is a global override intended for debugging and special builds.")
   

   happy to drop the “global override”, but “It is” was a bit imprecise, it’s not clear what the “It” refers to.

   ---

   

   hebasto commented at 3:15 pm on June 21, 2024:

   Reworked. Not using “global override” as it can be confusing in cases when libsecp is integrated by an upstream project.
5. 
   real-or-random commented at 3:09 pm on June 21, 2024: contributor

   ACK mod nit
6. real-or-random added the label build on Jun 21, 2024
7. cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach 4706be2cd0
8. hebasto force-pushed on Jun 21, 2024
9. real-or-random approved
10. 
    real-or-random commented at 4:09 pm on June 21, 2024: contributor

    utACK 4706be2cd0bcac97da10ed3b98790f1ac6f04efb
11. 
    hebasto commented at 4:14 pm on June 21, 2024: member

    Friendly ping @theuni :)
12. 
    fanquake commented at 9:18 am on June 25, 2024: member

    Concept ACK. Aligning the naming is nice and this implementation is simpler. I think “special builds” is ambiguous, and downstreams may likely (mis)use this option in any case, but I can’t really think of anything better to call it.
13. real-or-random merged this on Jun 25, 2024
14. real-or-random closed this on Jun 25, 2024

    ---

15. hebasto deleted the branch on Jun 25, 2024

Contributors
hebasto real-or-random fanquake

Labels
build