godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.74k stars 575 forks source link

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

Open MartinStimpfl opened 6 months ago

MartinStimpfl commented 6 months ago

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 commented 1 month ago

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.