search

bitcoin-core / secp256k1

Optimized C library for EC operations on curve secp256k1
MIT License
2.06k stars 1k forks source link

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

Closed hebasto closed 3 months ago

hebasto commented 3 months ago

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?

hebasto commented 3 months ago

@real-or-random

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

hebasto commented 3 months ago

Friendly ping @theuni :)

fanquake commented 3 months ago

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.