Broken logic in MSVC runtime selection?

[jcelerier]

Godot version

4.5

godot-cpp version

4.5

System information

Windows w/MSVC

Issue description

The logic about the CRT in windows.cmake seems flawed: it requires users to add new build flags to counteract the setting of CMAKE_MSVC_RUNTIME_LIBRARY when including godot-cpp somewhere in their project versus just letting the CMake defaults.

As a CMake user, I really don't want libraries I include to fiddle with global CMake options, this is the responsibility of the end-user to do this explicitly ; doing otherwise really breaks expectations.

To keep the GODOTCPP_USE_STATIC_CPP, what I would expect instead here would be:

set(CMAKE_MSVC_RUNTIME_LIBRARY
    "MultiThreaded$<$<CONFIG:Debug>:Debug>$<$<NOT:$<BOOL:${GODOTCPP_USE_STATIC_CPP}>>:DLL>"
    CACHE STRING
    "Select the MSVC runtime library for use by compilers targeting the MSVC ABI."
)

which will correctly keep Debug builds with the Debug CRT, and support static-debug build (/MTd), e.g. the normal expectation when building in the MSVC ecosystem, unlike what the code currently does which is forcing the Release CRT unless the user sets GODOTCPP_DEBUG_CRT, thus breaking build in a given build system that is not godot-related (as noticed and mentioned in the last paragraph in the cmake file!).
Since godot has a C ABI between DLLs and the engine, this as far as I know should not cause a problem (and also enable more useful debug checks for gdextension autors).

Or at worst, provide a godot_setup_gdextension(TARGET) function that end-users have to call, and which will set MSVC_RUNTIME_LIBRARY on targets that link to godot-cpp without leaking into the rest of a given build system

Steps to reproduce

add_subdirectory(godot-cpp)
add_library(foo SHARED foo.cpp) #oops, cmake --build . --config Debug on MSVC produces foo with the release crt

Minimal reproduction project

cmake_minimum_required(VERSION 4.0 FATAL_ERROR)
project(repro)

include(FetchContent)
FetchContent_Declare(
      godot-cpp
      GIT_REPOSITORY  https://github.com/godotengine/godot-cpp.git
      GIT_TAG         4.5
      GIT_SHALLOW     TRUE
)
FetchContent_MakeAvailable(godot-cpp)

file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/foo.cpp" "int main(){}")
add_library(foo SHARED "${CMAKE_CURRENT_BINARY_DIR}/foo.cpp") 

[enetheru]

Yeah that was a massive pain to try to get right and try to maintain parity with the SCons solution.
I'd be happy to endorse a PR to change it.
What are if any the likely impacts on existing extensions?
This would remove the GODOTCPP_DEBUG_CRT option entirely yes?
I'm guessing the stuff I wrote that attempted to explain my reasoning would also need to be changed.

[jcelerier]

Hmm I think the core thing is: the expectations cmake users have are fairly (wildly) different from the expectations around scons ; different programs, yada yada. Having participated in the port of multiple large build systems to cmake, definitely everything is much smoother for the end-user when the build system does not impose too many things on the end-user (at the cost of the end-user having to be more explicit with the options they pass; I think a very good way to handle this nowadays is CMake presets).

That said, for sure breaking changes are also not something enjoyable. I think the best would be to have the opinion of multiple godot-cpp users.

What are if any the likely impacts on existing extensions?

• no existing build folder: debug builds will use by default the debug crt and debug mode flags (so iterator checking, etc., which may make things slower in debug mode)
• existing build folder: since it's a cache variable I believe it would not change.

This would remove the GODOTCPP_DEBUG_CRT option entirely yes?

yep!

I'm guessing the stuff I wrote that attempted to explain my reasoning would also need to be changed.

for sure

[enetheru]

I'll just tag a bunch of people who may be able to provide some strength to the case.
@Naros @vorlac @foxandfeature @ytnuf @Ughuuu

I'm in favor of changing it, I just don't have the breadth of experience to know if there will be any pitfalls.