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

GDExtension Bindings libraries built with SCons and CMake differ when built with MSVC #1459

Closed
MartinStimpfl opened this issue May 8, 2024 · 5 comments

Comments

@MartinStimpfl
Copy link

Godot version

N/A

godot-cpp version

4.2.2 stable

System information

Windows 10

Issue description

Building the Godot-cpp Bindings via the provided SCons and CMake setups in Windows with MSVC result in .libs with different configurations or at least built with different compiler flags. The SCons version seems to always have the /MT flag set (for release and debug builds) and the CMake version the /MD flag for release builds and the /MDd flag for debug builds.
This results in compile errors like
libgodot-cpp.windows.template_debug.x86_64.lib(error_macros.windows.template_debug.x86_64.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MT_StaticRelease' doesn't match value 'MDd_DynamicDebug' in gdexample.obj
when linking the bindings library built with SCons in debug with a GDExtension project that has not /MT explicitly set.
Since in the CMake file /MD and /MDd are explicitly set for release and debug builds respecivly, I assume this behaviour was not intended.

Other compilers or OSs were not tested regarding this issue.

Steps to reproduce

Follow the instructions to get and build the Godot-Bindings and the GDExtension example from the C++ tutorial.
When both the bindings and the example are built with SCons it compiles without problem.

When using a CMakeLists.txt in the root directory like

cmake_minimum_required(VERSION 3.18)

set(CMAKE_CXX_STANDARD 17)

project(GDExample)

add_library(gdexample SHARED
    src/gdexample.cpp
    src/register_types.cpp
)

target_include_directories(gdexample PUBLIC
    ${CMAKE_SOURCE_DIR}/godot-cpp/gdextension
    ${CMAKE_SOURCE_DIR}/godot-cpp/include
    ${CMAKE_SOURCE_DIR}/godot-cpp/gen/include
)

target_compile_options(gdexample PUBLIC
    # /MD
    # /MT
)

target_link_libraries(gdexample
    # ${CMAKE_SOURCE_DIR}/godot-cpp/bin/godot-cpp.windows.debug.64.lib
    ${CMAKE_SOURCE_DIR}/godot-cpp/bin/libgodot-cpp.windows.template_debug.x86_64.lib
)

to build the example with, compile errors like the one shown above should appear. Also when using /MD, /MDd or /MTd flags.
When commenting in the /MT compiler flag, the errors are gone.

When building the binding with CMake (debug build; as per example in the CMakeLists.txt in the bindings repository)

cd godot-cpp
mkdir build
cd build
cmake build ..
cmake --build .

and linking the resulting library with the GDExtension example (e.g. commenting out the SCons-.lib and commenting in the CMake-.lib in my CMakeLists.txt example) the errors appear only if the /MT or /MTd flags are set.

This should show, that the bindings library configuration differs between the provided SCons and the CMake setups.

Minimal reproduction project

N/A

@enetheru
Copy link
Contributor

As part of my work to modernise the cmake solution I have performed analysis on the actual command output of the build systems and there is dramatic variability between the cmake and the scons solution. See here for a comparison table.

People who use CMake (myself included) expect that all of the transient dependencies to be defined in the projects they consume. So using a command like target_link_libraries( my_project godot-cpp) will automatically set the includes, defines, compile flags, and link flags based on the options specified by the user. In the current cmake solution this is not the case and its a bit of a whack a mole to figure out what is propagated and what isnt.

In the ideal scenario the consumed library(godot-cpp) is compiled and linked to your project as a build time dependency using the options you specify, not compiled separately and then linked. This is also the method scons uses, that is to build godot-cpp as part of your project, not separarely and then linked.

I believe, that if my PR's get merged, then this issue will be solved.

Just an FYI, Visual Studio is multi-config generator and ignores -DCMAKE_BUILD_TYPE=Release. Instead this needs to be specified in the build command like cmake --build . --config Release For the current master branch, there are variables that rely on -DCMAKE_BUILD_TYPE=Release. so currently needs to be specified in both the configure step, and the build step if you want to build a different config.

@enetheru
Copy link
Contributor

This issue is still relevant, the scons solution does not link to the MTd and MDd variants if debug symbols are enabled using dev_build.

Which means that they arent interchangable at this moment, should I change the cmake code?or should I change the scons code?

I'm thinking at this moment that I change the cmake to match the scons, and if its needed in the future then a flag can be introduced like the godot engine has in platforms/windows/detect.py:459

enetheru added a commit to enetheru/godot-cpp that referenced this issue Nov 23, 2024
SCons does not link to /MDd or /MTd
If needed in the future a flag can be introduced like the engine code at platforms/windows/detect.py:459
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 25, 2024

Per this comment from bruvzg, I think we update CMake to match scons (which is what it looks like your PR is doing)

@enetheru
Copy link
Contributor

enetheru commented Dec 9, 2024

@dsnopek Now #1663 merged, this Issue can be closed.

Flags match the scons solution, including the two matching debug_crt and GODOT_DEBUG_CRT options for both build systems.

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 9, 2024

Thanks!

@dsnopek dsnopek closed this as completed Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants