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: Fix selection of MSVC Runtime compile flags #1663

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

enetheru
Copy link
Contributor

@enetheru enetheru commented Nov 30, 2024

My last attempt at solving this was not correct, and I apologise for that.

MSVC Runtime Selection

There are two main ways to set the msvc runtime library; Using target_compile_options() to add the flags or using the CMAKE_MSVC_RUNTIME_LIBRARY property abstraction1, introduced in CMake version 3.15 with the policy CMP00912 to
remove the flags from CMAKE_<LANG>_FLAGS_<CONFIG>.

Default: CMAKE_MSVC_RUNTIME_LIBRARY="MultiThreaded$<$<CONFIG:Debug>:Debug>DLL"

This initializes each target's MSVC_RUNTIME_LIBRARY property at the time of target creation.

This creates a conundrum for us, the CMAKE_MSVC_RUNTIME_LIBRARY needs to be correct at the time the target is created, but we have no control over the consumers CMake scripts, and the per-target MSVC_RUNTIME_LIBRARY property
is not transient, that is, it doesn't flow to dependent targets.

The previous attempt using INTERFACE_MSVC_RUNTIME_LIBRARY was a complete brain fail on my part, the INTERFACE_ prefix is not a universal concept like I thought it was, this property will do nothing.

We need CMAKE_MSVC_RUNTIME_LIBRARY to be "$<1:>"3 to ensure it will not add any flags. And then use target_compile_options() so that our flags will propagate to consumers.

In the interests of playing nicely we detect whether we are being consumed and notify the consumer that we are setting CMAKE_MSVC_RUNTIME_LIBRARY, that dependent targets rely on it, and point them to these comments as to why.

Further information from the cmake forum

Footnotes

  1. https://cmake.org/cmake/help/latest/policy/CMP0091.html

  2. https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html

  3. This is a generator expression that results in the empty string.

@enetheru enetheru force-pushed the fix_crt_debug branch 2 times, most recently from 8a743fc to fa1497a Compare November 30, 2024 09:24
@enetheru
Copy link
Contributor Author

OK, to verify that I have this one right this time, I have compiled the test project, on windows with the following configurations.

scons verbose=yes target=template_debug

scons verbose=yes target=template_debug debug_crt=yes

scons verbose=yes target=template_debug use_static_cpp=no

cmake --fresh ..
cmake --build . --verbose -t godot-cpp-test --config Release

cmake --fresh .. -DGODOT_DEBUG_CRT=YES
cmake --build . --verbose -t godot-cpp-test --config Release

cmake --fresh .. -DGODOT_USE_STATIC_CPP=NO
cmake --build . --verbose -t godot-cpp-test --config Release

Cleaned up the compile logs, and compared them in this spreadsheet:
https://docs.google.com/spreadsheets/d/1PC_5-zkCpUHSSWhS2OfSPLHctvTQxCRDPq_ogpmmXmQ/edit?usp=sharing

While there are still discrepancies between the builds, they are down to other factors.

I've also got an external project replicating the test folder, but entirely separated from the sources so it has to fetch and consume godot-cpp independently. This also correctly picks up the options.

@enetheru enetheru marked this pull request as ready for review November 30, 2024 09:49
@enetheru enetheru requested a review from a team as a code owner November 30, 2024 09:49
@dsnopek dsnopek added bug This has been identified as a bug cmake labels Dec 2, 2024
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

@dsnopek dsnopek added this to the 4.x milestone Dec 2, 2024
My last attempt at solving this was not correct. I have left lots of comments in the source detailing the issue as it will effect consumers.
@dsnopek dsnopek merged commit 4eaef4c into godotengine:master Dec 6, 2024
11 checks passed
@enetheru enetheru deleted the fix_crt_debug branch December 6, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug cmake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants