cmake: Improve handling of SECP256K1_APPEND_*FLAGS options
#1648
Conversation
|
Can you indicate why this is a draft? |
I haven't tested this branch with Xcode on macOS yet. |
hebasto
force-pushed
the
241213-append
branch
from
debc50d to
ce23e6e
Compare
Reworked to support only command-line build tool generators. Undrafted and ready for review :) |
There was a problem hiding this comment.
On the master branch @ f79f46c, setting
SECP256K1_APPEND_*FLAGSwhen 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_*FLAGSoptions 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.
CMakeLists.txt
Outdated
| if(CMAKE_GENERATOR MATCHES "Make|Ninja") | ||
| # Only command-line build tool generators support low-level rule | ||
| # variables. |
There was a problem hiding this comment.
nit:
While this seems to be true, I think it's a coincidence that these happen to be the command-line tools.
| if(CMAKE_GENERATOR MATCHES "Make|Ninja") | |
| # Only command-line build tool generators support low-level rule | |
| # variables. | |
| # Only some generators use low-level rule variables. | |
| if(CMAKE_GENERATOR MATCHES "Make|Ninja") |
And I think the command should be above the if line because it comments on the if.
| @@ -287,7 +287,7 @@ if(CMAKE_GENERATOR MATCHES "Make|Ninja") | |||
| # guarantee that the flags appear at the end of the command line. | |||
| 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.") | |||
| if(SECP256K1_APPEND_CFLAGS) | |||
| string(APPEND CMAKE_C_COMPILE_OBJECT " ${SECP256K1_APPEND_CFLAGS}") | |||
| string(REPLACE "<FLAGS>" "<FLAGS> ${SECP256K1_APPEND_CFLAGS}" CMAKE_C_COMPILE_OBJECT "${CMAKE_C_COMPILE_OBJECT}") | |||
There was a problem hiding this comment.
Do you think this same should be done for CMAKE_C_CREATE_SHARED_LIBRARY and/or CMAKE_C_LINK_EXECUTABLE?
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.
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.
hebasto
force-pushed
the
241213-append
branch
from
ce23e6e to
c12bc01
Compare
On the master branch @ f79f46c, setting
SECP256K1_APPEND_*FLAGSwhen 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_*FLAGSoptions 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):
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.