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: Use proper build configuration during compiler/linker flag tests #285

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jul 25, 2024

This PR makes all compiler/linker flag tests use the following build configuration:

  • CMAKE_BUILD_TYPE with single-config generator, which means the same configuration during tests and the build.
  • the default configuration, which is "RelWithDebInfo", with multi-config generators.

Addresses the issue raised during today's CMake-WG call.

@hebasto
Copy link
Owner Author

hebasto commented Jul 25, 2024

cc @fanquake @TheCharlatan @theuni

@hebasto hebasto added the bug Something isn't working label Jul 25, 2024
@theuni
Copy link

theuni commented Jul 30, 2024

Lol. See comment here: #282 (comment)

I just came to the exact same conclusion :)

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK other than the doc request.

@@ -74,6 +74,7 @@ function(set_default_config config)
if(is_multi_config)
get_property(help_string CACHE CMAKE_CONFIGURATION_TYPES PROPERTY HELPSTRING)
set(CMAKE_CONFIGURATION_TYPES "${all_configs}" CACHE STRING "${help_string}" FORCE)
set(CMAKE_TRY_COMPILE_CONFIGURATION "${config}" PARENT_SCOPE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a link to this here? This is arguably a bug that will likely be fixed in the future.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

@m3dwards
Copy link

m3dwards commented Jul 31, 2024

ACK 60d061a

Before PR with single config generators different flags were passed to TRY_COMPILE steps and after PR they matched. With multi config generators (I tested with Ninja) and before PR applied, no flags associated with build type were used with TRY_COMPILE steps. After PR - flags associated with RelWithDebInfo were seen.

It is worth noting that there is an (unavoidable?) subtle difference now between single config generator and multi config generators in that single config generators will align TRY_COMPILE flags with the actual compile flags. Whereas multi config generators will use different flags at compile time depending on build type selected and always flags associated with RelWithDebInfo for configuration time TRY_COMPILE checks.

@hebasto
Copy link
Owner Author

hebasto commented Jul 31, 2024

Rebased due to the recent sync/rebase cycle.

@m3dwards
Copy link

reACK e51c781

@theuni
Copy link

theuni commented Jul 31, 2024

It is worth noting that there is an (unavoidable?) subtle difference now between single config generator and multi config generators in that single config generators will align TRY_COMPILE flags with the actual compile flags. Whereas multi config generators will use different flags at compile time depending on build type selected and always flags associated with RelWithDebInfo for configuration time TRY_COMPILE checks.

Agree this is annoying, but it's unfortunately just how CMake works. The lead dev says as much in the issue linked above. Ideally multi-config would do checks for each type (and even more ideally, in parallel), but this is just how it is for now I'm afraid.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK. I haven't tested, but this is what I expected.

@hebasto hebasto merged commit ef16073 into cmake-staging Aug 5, 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.

3 participants