{cmake} Use cmake capabilities

[asmaloney]

• (gcc/clang) should not set "-g" (debug symbols) explicitly; this is controlled by CMAKE_BUILD_TYPE for single-configuration generators (Debug or RelWithDebInfo will include symbols)
• (gcc/clang) "-O0" is the default for Debug builds
• (gcc/clang) "-O3" is the default for Release builds
• (MSVC) remove explicit setting of flags which are already set by Debug/Release (/EHsc, /Md, etc.)
• (MSVC) remove manipulation of CMAKE_CXX_FLAGS_DEBUG; the including project should control this
• remove/replace references to CMAKE_BUILD_TYPE to allow for multi-config generators

- we are building a static lib, so don't set the link flags explicitly, use STATIC
- use the POSITION_INDEPENDENT_CODE property instead of setting the flag explicitly
(Moved to its own PR.)

[asmaloney]

FYI I'm trying to get this CMake to play nicer with CMake-based projects which include this as a subdirectory (e.g. GDExtensionTemplate). The including project should have control over optimization level and debug symbols, for example.

Also - I'm not sure about the Windows stuff here (since I don't build on Windows) - but I suspect that some of it is setting things that are already defaults. This can can probably be cleaned up as well.

[Calinou]

Out of curiosity, does enabling LTO/LTCG with -DINTERPROCEDURAL_OPTIMIZATION=1 work with godot-cpp?

[asmaloney]

Following up on my previous comment, I used CI to look at the command lines being produced by MSVC and removed some redundancies from the CMake file.

[asmaloney]

Out of curiosity, does enabling LTO/LTCG with -DINTERPROCEDURAL_OPTIMIZATION=1 work with godot-cpp?

Oh! I'm not sure. I've not used INTERPROCEDURAL_OPTIMIZATION before, but I can try it locally (macOS x86_64).

[Calinou]

I may have gotten the name wrong – it might be -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=1 actually.

[asmaloney]

👍 It's INTERPROCEDURAL_OPTIMIZATION on the target though.

[asmaloney]

INTERPROCEDURAL_OPTIMIZATION is doing some magic on macOS. It reduced the resulting size of the final .dylib by half (both debug & release - universal builds).

Still loads and seems to run fine in Godot.

[mihe]

Note that after this PR the CI builds for Visual Studio will be Release in name only, and will instead build (the default) non-optimized builds, since they're (incorrectly) treating the Visual Studio generator as a single-config generator.

I tried addressing this in my (admittedly over-complicated) PR #893.

The very abbreviated version of it is that CMAKE_BUILD_TYPE should not be used anywhere if you aim to support multi-config generators like Visual Studio or Xcode, as seen in CMake's documentation.

I would suggest replacing any remaining occurrence of CMAKE_BUILD_TYPE left in CMakeLists.txt with the appropriate generator expression and also change the CI builds for Visual Studio to not pass -DCMAKE_BUILD_TYPE=... and instead run cmake --build --config Release.

[asmaloney]

Thanks @mihe - I will look at it!

[asmaloney]

Updated based on input from @mihe.

• remove/replace references to CMAKE_BUILD_TYPE to allow for multi-config generators

[asmaloney]

Would it be easier if I broke this PR up a bit? I can see about 3 logical pieces I can break it into.

[akien-mga]

I'm not deeply familiar with CMake, but I have one requirement for this buildsystem: The output should be the same as the SCons reference buildsystem.

The CMake tooling is provided as convenience for people who want to integrate in other CMake projects, or who really don't want to use SCons for some reason, but the generated binaries for equivalent options in SCons and CMake should be the same.

As such, I'm not too fond of removing explicit flags because they're default in CMake's equivalent config. Godot's flags may change in the future, or CMake's flags may change. Both should always be in sync.

Explicit is always better than implicit so that we know what we're doing and why. This isn't a decision we're leaving to the CMake team.

[asmaloney]

I get where you are coming from, but anyone familiar with CMake and trying to use this in their own project will find it difficult (or impossible) to work with. There are several anti-patterns here (e.g. setting some flags like -g directly, some flags should not be set by libraries, regex-ing to modify compiler flags, the reliance on CMAKE_BUILD_TYPE brought up earlier, etc.). This file should set its defaults and hard requirements (c++17 for example), but the including project should be in control when including a library and the way to do that is to use CMake properly.

I would argue that godot-cpp is not Godot - it's the API a non-godot developer uses to write their extensions and therefore it needs to integrate with their code and existing libraries - not the other way around. It's essentially the UX for someone trying to integrate with Godot and as such it should follow standards (actual & defacto) more closely to make it easier to understand and easier to work with for the (non-godot) developers.

With all the PRs/issues I've made in godot-cpp I've tried to adopt the viewpoint of a C++ developer without any knowledge of Godot's code base who wants to create an extension for Godot. I ask questions such as "how would they expect this to work?", "how does this work if they are integrating one or more existing CMake-based libraries? (e.g. conflicting explicit flags?)", "can this be made clearer & easier?", ""are there problems what would prevent them from working with it?". Then I worked through the experience by building the GDExtensionTemplate project (and another project using it - Inception?) to provide a solid starting point.

I'm trying to improve the dev's UX because I believe extensions are a critical part of Godot's ecosystem, so making it easy & familiar is a big win.

All that said, most of the changes here are not making explicit flags implicit. I will pick this PR apart and try smaller PRs so we can see where the friction points are.

[asmaloney]

I've just received my first report about linking issues that seems the be because godot-cpp is trying to do things its own way. Those MSVC link options have to be consistent throughout the build, so setting them explicitly just won't work.

The way things are right now, we can't set up multiconfig builds using godot-cpp (see discussion about DCMAKE_BUILD_TYPE above). This will need to be addressed one way or another so that projects can link this properly.

[asmaloney]

(Force push to remove STATIC and POSITION_INDEPENDENT_CODE - those were moved to their own PR.)

[mihe]

Those MSVC link options have to be consistent throughout the build, so setting them explicitly just won't work.

Yeah, the MSVC runtime library stuff (assuming you're talking about asmaloney/GDExtensionTemplate#43) was part of the PR I linked above.

This is two problems rolled up into one.

The first problem is, as you say, the reliance on CMAKE_BUILD_TYPE, where godot-cpp assumes a single-config generator and defaults CMAKE_BUILD_TYPE to Debug when none is provided, which shouldn't be provided when generating Visual Studio solutions, leading to /MDd (debug DLL runtime) being used across all configurations, including Release, which should be using /MD (non-debug DLL runtime) in this case. MSVC does not allow you to mix these within the same binary.

The second problem is that even if the first problem wasn't a thing, you'd still run into this linker error when a user wants to statically link against MSVC's runtime, meaning /MTd or /MT. You're not allowed to mix /MT[d] and /MD[d] within the same binary, so you need some way to configure this. Godot core provides a build option for this, but godot-cpp does not currently.

This is something you find in CMake setups of other popular libraries as well, such as GLFW. I'm not sure if there's much of an argument for making it a build option with CMAKE_MSVC_RUNTIME_LIBRARY being a thing nowadays, but keep in mind that CMAKE_MSVC_RUNTIME_LIBRARY was introduced in CMake 3.15 and godot-cpp is still targeting 3.12 as a minimum.

[enetheru]

How much of this conversation is still relevant?

• I've bumped the cmake minimum version to 3.17 due to requirements for Emscripten.
• We are still setting the /MD /MT option manually though that can now change since the version bump
• We dont rely on CMAKE_BUILD_TYPE at all anymore.
• I'm interested in adding the INTERPROCEDURAL_OPTIMIZATION to the solution thats new to me.