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 rewrite #1355

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

IvanInventor
Copy link

Complete CMake rewrite. Main targets:

  • Replicate SCons build variables as much as possible, except choosing build tool part, which is handled by CMake toolchains provided by tool vendors themselves (Android NDK, Emscripten etc.). Some custom toolchains still can be made, but should be separated from main config and placed, for example, in /cmake/toolchains/***.
  • Platform-specific options are also handled similar to SCons with /cmake/godotcpp.cmake file containing global variables, and /cmake/<platform-name>.cmake with platform-specific variables.
  • Experimental option to compile godot-cpp as shared library for potential benefit of having multiple GDExtension projects to reuse single shared library. CMake handles linking to shared library by default, but not rpath, so additional configuration is needed.

Test project changes:

  • Test's CMakeLists.txt rewrite to make use of godot-cpp's options and godot-cpp target's public include header paths.
  • Added empty export preset that fixes problem of needing to open project in editor at least once to use GDExtension libraries.

In result, both /CMakeLists.txt and /test/CMakeLists.txt are much cleaner, with all configuration handled by modules in /cmake, and all configuration in /test inherited with add_subdirectory().

Tested with all tests finished on:
Linux gcc x86_64
Linux clang x86_64
Windows mingw x86_64
Web Emscripten wasm32
Android arm64

TODO list:

  • MacOS/IOS configurations are made, but not tested, and probably need some changes specifially to "framework" part, which is, as i understand CMake reference, should be handled by FRAMEWORK property when compiling on Mac, or with some directories/files created, if not.

How to add auto-detection for custom toolchain:

  • Set default platform name based on CMAKE_SYSTEM_NAME variable set by toolchain at /cmake/godotcpp.cmake # Auto-detect platform line.

@Calinou Calinou added the enhancement This is an enhancement on the current functionality label Jan 9, 2024
CMakeLists.txt Outdated
#
# Todo
# Test build for Windows, Mac and mingw.

cmake_minimum_required(VERSION 3.12)
cmake_minimum_required(VERSION 3.24)
Copy link
Member

Choose a reason for hiding this comment

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

What are the changes that requite 3.24? We should use the lowest possible version to maintain stability and support, people shouldn't be forced to update unless some specific feature is required

Copy link
Member

@Calinou Calinou Jan 9, 2024

Choose a reason for hiding this comment

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

Indeed, many people work with LTS Linux distributions that only package older CMake versions (or they rely on the CMake version installed by Visual Studio).

I suggest not requiring a version newer than 3.16, since that's what Ubuntu 20.04 has (Ubuntu 20.04 is supported until April 2025).

Copy link
Author

Choose a reason for hiding this comment

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

I'll revert to old versions in later updates

Copy link
Author

Choose a reason for hiding this comment

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

What are the changes that requite 3.24? We should use the lowest possible version to maintain stability and support, people shouldn't be forced to update unless some specific feature is required

it was made to support single COMPILE_WARNING_AS_ERROR ON property, but i already have version issues on VS 2019, so i'll revert it.

@dsnopek
Copy link
Contributor

dsnopek commented Jan 10, 2024

Thanks!

The cmake configuration really could use a rewrite to be closer to what we have in scons, both functionally and structurally, which is what it looks like your PR is aiming to do. Unfortunately, since I don't know cmake very well, I may not be the best person to evaluate if this PR meets that goal. :-)

Experimental option to compile godot-cpp as shared library for potential benefit of having multiple GDExtension projects to reuse single shared library. CMake handles linking to shared library by default, but not rpath, so additional configuration is needed.

If we want to support this, it should be supported in scons as well. In general, I don't think we should have anything in cmake that we don't also have in scons, unless it's something particular to a cmake workflow.

But, also, I'm not sure we want to support this. While I guess this feature could make sense if you have multiple GDExtensions that are specific to your game project (and hence not shared between projects), I worry that this will just lead to symbol conflicts or the dynamic linker re-linking symbols in an unexpected way, when combining with GDExtensions from other sources.

I'd be curious to hear what other folks think, though!

@IvanInventor
Copy link
Author

Updates:

  • Cmake version revert
  • Proper single and multi-config support
  • Default Cmake configuration types, godot targets (editor, template_debug, template_release) as separate TARGET variable
  • SCons optimization levels are mapped to default Cmake configuration types (Debug, Release, RelWithDebInfo, MinSizeRel) and available from single build directory for multi-config generators (Ninja Multi-Config, Visual Studio), see notes in /CMakeLists.txt
    OPTIMIZE, DEBUG_SYMBOLS variables still exist to force specific compile flags
  • Cmdline examples fix based on changes
  • Output path for multi-config fix, so it doesn't create subdirectories but replace final shared library
  • Default CXX flags are cleared (fully replaced by ours)
  • Public definitions fix (was private, caused errors)

Full list of tested platforms and cmdline commands (via godot-containers)

  • Linux
    • GNU x86_64
      cmake --toolchain $GODOT_SDK_LINUX_X86_64/share/buildroot/toolchainfile.cmake -G "Ninja Multi-Config" -Bbuildx86_64 && cmake --build buildx86_64
  • Windows
    • MinGW x86_64
      cmake --toolchain /usr/share/mingw/toolchain-mingw64.cmake -G "Ninja Multi-Config" -Bbuildx86_64 && cmake --build buildx86_64
    • MSVC 2019 x86_64 (vm, not godot-container)
      cmake -G "Visual Studio 16 2019" -A x64 -Bbuildx86_64 && cmake --build buildx86_64
  • Web
    • Emscripten wasm32
      cmake --toolchain /root/emsdk/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake -G "Ninja Multi-Config" -Bbuildweb && cmake --build buildweb
  • Android
    • Android NDK arm64
      cmake --toolchain $ANDROID_NDK_ROOT/build/cmake/android.toolchain.cmake -G "Ninja Multi-Config" -Bbuildandroid -DANDROID_PLATFORM=21 && cmake --build buildandroid

Mac/IOS support

All needed variables are replicated, but there's no way for me to build/test it without their toolchain, so i plan to leave warning message there and let someone else try to build it and modify, if needed, as any other platform is pretty much ready.

@IvanInventor
Copy link
Author

IvanInventor commented Jan 11, 2024

Experimental option to compile godot-cpp as shared library for potential benefit of having multiple GDExtension projects to reuse single shared library. CMake handles linking to shared library by default, but not rpath, so additional configuration is needed.

If we want to support this, it should be supported in scons as well. In general, I don't think we should have anything in cmake that we don't also have in scons, unless it's something particular to a cmake workflow.

It's a feature that works as static library by default, but when someone actually needs it, it will be a matter of setting single variable and one link flag -Wl,-rpath,<relative_path> outside of godot-cpp's script, so no fork for adding single option will be needed

I worry that this will just lead to symbol conflicts or the dynamic linker re-linking symbols in an unexpected way, when combining with GDExtensions from other sources.

When godot-cpp's Cmake config will have every option it needs to compile, the same options can be used by subprojects using godot-cpp as dependency, and modifying them to either use their provided godot-cpp in submodule, or godot-cpp from main project, may be as easy as adding this conditional to their CMakeLists.txt

if(TARGET godot-cpp)
	# godot-cpp target already exists from top project
	# do nothing
else()
	# godot-cpp from project's submodules
	add_subdirectory(godot-cpp)
endif()
# At this point godot-cpp is either target created by main project
# (reused for every subproject), or godot-cpp from project's submodules
add_library(project_library ...)
target_link_libraries(project_library PUBLIC godot-cpp)

@IvanInventor IvanInventor force-pushed the cmake-rewrite branch 2 times, most recently from 796d4c9 to a37226e Compare January 13, 2024 22:05
@IvanInventor
Copy link
Author

Small final updates

  • /test's config now can be easily used as a template for custom project with single GODOT_CPP_PATH modification
  • setting CMAKE_CONFIGURATION_TYPES works in top project's script only, so test-building both godot-cpp and gdexample in CI can be made from /test with
cmake -Bbuild # Configure
cmake --build build -- godot-cpp # godot-cpp only
cmake --build build # gdexample
  • build command example for every platform

At this point cmake builds work on every platform/arch combination (tested via toolchains provided on godot-containers build), except Mac/IOS, where it's not tested

@IvanInventor IvanInventor marked this pull request as ready for review January 13, 2024 22:28
@IvanInventor IvanInventor requested a review from a team as a code owner January 13, 2024 22:28
Copy link

@enetheru enetheru left a comment

Choose a reason for hiding this comment

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

Looks like a valid re-write to match the scons scripts in spirit. and a better starting point for further changes.

@fire fire requested a review from dsnopek March 10, 2024 00:12
@enetheru
Copy link

enetheru commented Mar 10, 2024

@dsnopek

I may not be the best person to evaluate if this PR meets that goal. :-)

Unfortunately, it looks like nobody on the gdextension team is very experienced with cmake, leaving skills shortage. Luckily you have a few contributors who can help like @asmaloney @vorlac @Naros

@dsnopek
Copy link
Contributor

dsnopek commented Mar 10, 2024

Thanks! It'd be really great to get some more review on this, especially from folks who depend on the current cmake build, to ensure that it will still work for their use cases.

I think what we really need long-term is someone to maintain the cmake build in godot-cpp, who we can trust to ensure
that it mirrors the SCons build (ie. not adding features that don't exist in the SCons build) and who will stick around so it won't get out-of-sync later as the SCons build changes.

@vorlac
Copy link

vorlac commented Mar 16, 2024

I just started taking a close look at all of the cmake changes in this PR to help give this an extra pair of eyes.

If it ends up helping get this PR some additional test coverage, I set up a branch of my gde side project from a few months ago to default it's godot-cpp submodule to the branch associated with this PR, anyone can clone that here if they want to help test things on as many different platforms/machines as possible:
https://github.com/vorlac/godot-roguelite/tree/godot-cpp-cmake-overhaul

that project should be relatively straight forward for anyone to try building on different platforms. the wiki has a ton of tips/guides that explain how to build it. for the most part, as long as cmake, ninja, msvc/clang/gcc, and scons are all installed and in your system PATH, the cmake scripts that are included in that project should set everything up for you and configure the build - everything from submodule init, building the engine (which is unnecessary for this test, it'll only run it once), configuring & building all 2rd party deps with vcpkg (which is also setup from the vcpkg submodule), and configures/builds godot-cpp + the shared gdextension shared library.

So far, the project builds and runs great with this new cmake setup on windows (using msvc), and a full clean + build ran perfectly using both clang & gcc on my selfhosted archlinux build machine that runs the automated github actions for that repo.

you can see the build results here (as well as grab example cmake commands if anyone reading this wants to try the build, but call it from the command line instead of through vscode/vs2022):
https://github.com/vorlac/godot-roguelite/actions/runs/8304400512/job/22729994374

I'll keep looking over all of the changes, but everything I've seen so far looks really clean and is definitely closer aligned to the scons build with the new options that allow for more granular control of the build. It also seems to handle certain compilers and/or platforms in a more thorough and organized layout of scripts. overall it looks like a really nice improvement.

I think it'll be worth trying it out on as many platform & compiler permutations as possible just to make sure there aren't any edge cases hiding in there. If anyone owns a mac and wants to try this I the branch above should 🤞 build on MacOS, but may be limited to just arm64 architecture (only one or two macos users ended up working with me to get it building on their machines, but I don't own a mac so I can't help test on any apple platforms).

If anyone ends up trying this out and you run into issues, feel free to reach to me in this thread or on discord (i'm @orlac in there) in the gdextension channel of the godot community server.

Really nice job @IvanInventor 👏

@vorlac
Copy link

vorlac commented Mar 16, 2024

I just scanned through the build output of the linux build and I think this one warning might be new, but i'm not entirely sure. Not sure if -Wtype-limits was suppressed previously or if this is expected on this branch..

g++ version being used:
https://github.com/vorlac/godot-roguelite/actions/runs/8304400512/job/22729994374#step:4:115

g++ warning:
https://github.com/vorlac/godot-roguelite/actions/runs/8304400512/job/22729994374#step:6:57

clang seems to be OK with it and doesn't report any warnings, so it might just be something slightly inconsistent with the build using the gnu toolchain.

@whiletrue111
Copy link

is short what is the status fo the cmake right now ? can we create VSC++ solution using it ?
if yes can we have short tutorial on how ?
Thanks allot

@IvanInventor
Copy link
Author

IvanInventor commented May 3, 2024

Updates

  • Rebased onto fresh commit
  • Mac support complete (with caveats)
  • -shared godot-cpp library feature (same can be done with BUILD_SHARED_LIBS option)
  • GODOT_C_FLAGS rename (no idea why i did CC in first place)
  • All godot-cpp specific compile/link flags and also duplicated as godot-cpp target's properties (will be useful later)

At this point it compiles and works on every platform (probably on every arch) except IOS, since i have no instruments available to test it, current cmake config structure allows to easily add it later, so i'll leave it to someone who needs it and can test it.

Also, i started working at CMake CI in addition to existing SCons one #1452

MacOS support caveats

  • Generating CMake project with existing project/bin/***.framework/Resources failed because it tries to create symlink Resources -> Versions/Current/Resources, so i made a workaround that deletes Resources folder (probably not a big deal, new Info.plist generates anyway)
  • Xcode on MacOS works
cmake -G Xcode -B build-osx
cmake --build build-osx
  • Compiling from godot-osx container (osxcross toolchain) works
/root/osxcross/target/bin/x86_64-apple-darwin23.3-cmake -G "Ninja Multi-Config" -B build-osx
/root/osxcross/target/bin/x86_64-apple-darwin23.3-cmake --build build-osx
  • Trying non-regular build tools on MacOS (Ninja, for example) works partially. It creates .framework folder, Info.plist and compiles proper binary, but doesn't create proper symlinks, but since regular builders work as intended, i'll leave it to someone wanting to use it specifically.

This was referenced May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants