cmake: Improve handling of `SECP256K1_APPEND

hebasto commented at 11:30 am on December 13, 2024: member

On the master branch @ f79f46c70386c693ff4e7aef0b9e7923ba284e56, setting SECP256K1_APPEND_*FLAGS when using IDE build tool generators, such as “Visual Studio” and “Xcode”, has no effect because these generators do not utilise low-level rule variables when creating project files.

This PR disables SECP256K1_APPEND_*FLAGS options in such scenarios. CMake will issue a warning if any of them are set.

Additionally, this PR fixes a bug with the clang-cl compiler (see #1647):

0> cmake -B build -G Ninja -DCMAKE_C_COMPILER=clang-cl -DSECP256K1_APPEND_CFLAGS=/W4
1> cmake --build build
2...
3clang-cl: error: no such file or directory: '/W4'
4...

The last change is beneficial in its own right, as it makes the compile invocation string more natural by ensuring that flags do not follow source files.

real-or-random added the label build on Dec 13, 2024

real-or-random added the label refactor/smell on Dec 13, 2024

real-or-random commented at 3:10 pm on December 13, 2024: contributor

Can you indicate why this is a draft?

hebasto commented at 3:21 pm on December 13, 2024: member

Can you indicate why this is a draft?

I haven’t tested this branch with Xcode on macOS yet.

hebasto force-pushed on Dec 15, 2024

hebasto marked this as ready for review on Dec 15, 2024

hebasto commented at 1:17 pm on December 15, 2024: member

Can you indicate why this is a draft?

I haven’t tested this branch with Xcode on macOS yet.

Reworked to support only command-line build tool generators.

Undrafted and ready for review :)

gatleas17 approved

in CMakeLists.txt:285 in ce23e6ee2f outdated

279@@ -280,19 +280,21 @@ if(SECP256K1_BUILD_CTIME_TESTS)
280   unset(msan_enabled)
281 endif()
282 
283-set(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 intended for debugging and special builds.")
284-if(SECP256K1_APPEND_CFLAGS)
285-  # Appending to this low-level rule variable is the only way to
286+if(CMAKE_GENERATOR MATCHES "Make|Ninja")
287+  # Only command-line build tool generators support low-level rule
288+  # variables.

real-or-random commented at 10:27 am on December 16, 2024:

nit:

While this seems to be true, I think it’s a coincidence that these happen to be the command-line tools.

0# Only some generators use low-level rule variables.
1if(CMAKE_GENERATOR MATCHES "Make|Ninja")

And I think the command should be above the if line because it comments on the if.

hebasto commented at 2:22 pm on December 17, 2024:

Thanks! Reworked.

in CMakeLists.txt:289 in ce23e6ee2f outdated

286@@ -287,7 +287,7 @@ if(CMAKE_GENERATOR MATCHES "Make|Ninja")
287   # guarantee that the flags appear at the end of the command line.
288   set(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 intended for debugging and special builds.")
289   if(SECP256K1_APPEND_CFLAGS)
290-    string(APPEND CMAKE_C_COMPILE_OBJECT " ${SECP256K1_APPEND_CFLAGS}")
291+    string(REPLACE "<FLAGS>" "<FLAGS> ${SECP256K1_APPEND_CFLAGS}" CMAKE_C_COMPILE_OBJECT "${CMAKE_C_COMPILE_OBJECT}")

real-or-random commented at 10:27 am on December 16, 2024:

Do you think this same should be done for CMAKE_C_CREATE_SHARED_LIBRARY and/or CMAKE_C_LINK_EXECUTABLE?

hebasto commented at 2:22 pm on December 17, 2024:

Thanks! Done.

real-or-random commented at 10:32 am on December 16, 2024: contributor

On the master branch @ f79f46c, setting SECP256K1_APPEND_*FLAGS when using IDE build tool generators, such as “Visual Studio” and “Xcode”, has no effect because these generators do not utilise low-level rule variables when creating project files.

This PR disables SECP256K1_APPEND_*FLAGS options in such scenarios. CMake will issue a warning if any of them are set.

Concept ACK

Special casing generators is not elegant, but yeah, it’s probably a good idea. Otherwise, anyone who wants to use SECP256K1_APPEND_CFLAGS on an unsupported build system will waste hours trying to understand why this is the case.

cmake: Enable `APPEND_*FLAGS` only for command-line generators

Command-line build tool generators are the only ones that support
low-level rule variables. IDE build tool generators, such as "Visual
Studio" and "Xcode", do not utilize these variables when creating
project files.

This change ensures that CMake warns users if any of the
`SECP256K1_APPEND_*FLAGS` options are set while using an unsupported
generator.

0be7d3d39e

cmake: Fix passing `SECP256K1_APPEND_CFLAGS` to clang-cl

This change also makes the compile invocation string more natural by
ensuring flags do not follow source files.

Linker flags are also amended for consistency.

c12bc0163b

hebasto force-pushed on Dec 17, 2024

real-or-random approved

real-or-random commented at 8:33 pm on December 17, 2024: contributor

utACK c12bc0163b03210e6187906273c6063dd94aa284

Datis77re approved

cmake: Improve handling of SECP256K1_APPEND_*FLAGS options #1648

cmake: Improve handling of `SECP256K1_APPEND_*FLAGS` options #1648