-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
base: master
Are you sure you want to change the base?
Cmake rewrite #1355
Conversation
09a22bc
to
3d4d41e
Compare
CMakeLists.txt
Outdated
# | ||
# Todo | ||
# Test build for Windows, Mac and mingw. | ||
|
||
cmake_minimum_required(VERSION 3.12) | ||
cmake_minimum_required(VERSION 3.24) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. :-)
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! |
Updates:
Full list of tested platforms and cmdline commands (via godot-containers)
Mac/IOS supportAll 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. |
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
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 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) |
796d4c9
to
a37226e
Compare
Small final updates
cmake -Bbuild # Configure
cmake --build build -- godot-cpp # godot-cpp only
cmake --build build # gdexample
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 |
There was a problem hiding this 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.
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 |
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 |
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: 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): 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 Really nice job @IvanInventor 👏 |
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 g++ version being used: g++ warning: 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. |
is short what is the status fo the cmake right now ? can we create VSC++ solution using it ? |
…configuration types, cmdline example fix, output path for multi-config fix, clean default CXX flags, public definitions fix
4861eb1
to
47b6eec
Compare
Updates
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
cmake -G Xcode -B build-osx
cmake --build build-osx
/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
|
Complete CMake rewrite. Main targets:
/cmake/toolchains/***
./cmake/godotcpp.cmake
file containing global variables, and/cmake/<platform-name>.cmake
with platform-specific variables.Test project changes:
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 withadd_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:
FRAMEWORK
property when compiling on Mac, or with some directories/files created, if not.How to add auto-detection for custom toolchain:
CMAKE_SYSTEM_NAME
variable set by toolchain at/cmake/godotcpp.cmake
# Auto-detect platform
line.