Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Allow overriding flags in compiler/linker flag checks #282

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jul 25, 2024

The APPEND_{CPP,C,CXX,LD}FLAGS flags are now considered when checking compiler/linker flags.

Fixes #279.

@fanquake
Copy link

Reading https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_PLATFORM_VARIABLES.html, one of the first things it says is:

This variable should not be set by project code. It is meant to be set by CMake's platform information modules for the current toolchain, or by a toolchain file when used with CMAKE_TOOLCHAIN_FILE.

So I think this needs some documentation, or an explanation of what it's doing.

@hebasto
Copy link
Owner Author

hebasto commented Jul 25, 2024

Reading https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_PLATFORM_VARIABLES.html, one of the first things it says is:

This variable should not be set by project code. It is meant to be set by CMake's platform information modules for the current toolchain, or by a toolchain file when used with CMAKE_TOOLCHAIN_FILE.

So I think this needs some documentation, or an explanation of what it's doing.

All CMake checks are implemented using the try_compile() command, which internally creates a tiny temporary test project. This PR applies to that test project the same approach as has been used since #184.

Setting the non-cache CMAKE_TRY_COMPILE_PLATFORM_VARIABLES variable before the project() command ensures that it will be available in all contexts mentioned in https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_PLATFORM_VARIABLES.html. In that case, I can't see any drawbacks to not following "This variable should not be set by project code." statement.

@fanquake
Copy link

The APPEND_{CPP,C,CXX,LD}FLAGS flags are now considered

Note that this also isn't just append flags. DCMAKE_CXX_FLAGS_RELEASE weren't being included either.

@fanquake
Copy link

Note that this also isn't just append flags. DCMAKE_CXX_FLAGS_RELEASE weren't being included either.

Testing this branch it seems like they are also still being ignored? The test case from #279 is still broken.

@hebasto
Copy link
Owner Author

hebasto commented Jul 25, 2024

Note that this also isn't just append flags. DCMAKE_CXX_FLAGS_RELEASE weren't being included either.

Testing this branch it seems like they are also still being ignored? The test case from #279 is still broken.

It is possible to override only using APPEND_* flags.

@hebasto hebasto added the bug Something isn't working label Jul 25, 2024
This change allows overriding flags in compiler/linker flag checks.
The `APPEND_{CPP,C,CXX,LD}FLAGS` flags are now considered when checking
compiler/linker flags.
@hebasto
Copy link
Owner Author

hebasto commented Jul 26, 2024

An explaining comment has been added per @fanquake's offline request.

@theuni
Copy link

theuni commented Jul 30, 2024

I've been playing with this all day and it's.. a mess.

From my tests I don't see a better way to get the flags appended, so utACK that I guess :(

For @fanquake's complaint, it seems this is our problem: https://gitlab.kitware.com/cmake/cmake/-/issues/19512

I'm able to get build_type flags into the try_compiles by moving the default build type selection up and setting CMAKE_TRY_COMPILE_CONFIGURATION:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fe6eab74f01..27e96e5b278 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -73,6 +73,10 @@ set(CMAKE_CXX_EXTENSIONS OFF)

 list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)

+include(ProcessConfigurations)
+set_default_config(RelWithDebInfo)
+set (CMAKE_TRY_COMPILE_CONFIGURATION ${CMAKE_BUILD_TYPE})
+
 #=============================
 # Configurable options
 #=============================
@@ -457,9 +461,6 @@ else()
   )
 endif()

-include(ProcessConfigurations)
-set_default_config(RelWithDebInfo)
-
 # Redefine/adjust per-configuration flags.
 target_compile_definitions(core_interface_debug INTERFACE
   DEBUG

@hebasto Mind having a look at that? Maybe it'd make sense to set that flag as part of set_default_config instead? Either way, it seems it's necessary to move the default selection up towards the beginning of the file.

@hebasto hebasto merged commit 02afae4 into cmake-staging Jul 30, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake: Allow overriding/take into account user flags
3 participants