Closed enetheru closed 5 hours ago
Looks like I have to fix up the test case
Because of the changes, the CI stopped working, so I needed to overhaul the test/CMakeLists.txt to consider the changes, and reduced the cmake code dramatically.
Again to aid with verification, here is a google doc spreadsheet showing the differences in output.
If there's anything you'd like me to check please mention it.
I found some issues with my hasty conversion of the test project.
So it looks both like RPATH is not relevant to windows, and that clang and gcc differ in their flags to set it. since I dont currently have a linux or mac computer, perhaps @aaronfranke you might be able to get some compiler and link output for me that I can see what is listed when you compile? I think CMAKE will automatically do it using the target properties feature, but since I am currently on windows I cant tell.. I can get a linux machine up and running soon, but it might not be before I go on holiday. But I have no access to a mac.
I also tried compiling Godot Orchestrator with this PR, which is a big Godot project using CMake
I'll try to see whats up with orchestrator.
Ivan has provided me some feedback: https://gist.github.com/IvanInventor/339f686883ea184b632bad1fcd183462#gistcomment-5199373
After reviewing how orchestrator expected to include the headers, I made an adjustment which is in keeping with those expectations. The include path is now exported to consumers, rather than all the header files individually. To fix my compilation of orchestrator I only needed to remove the include of GodotCompilerWarnings as they are now automatically propogated. And update the bindings to the godot version I was compiling against. Though I'm waiting on a compile of the engine without DEV_ENABLED so that I can load up the resulting dll. @aaronfranke
@aaronfranke Success!, Yeah so line 115, remove the INCLUDE( GodotCompilerWarnings ) is the only change I had to make. I have orchestrator example project working on godot/4.3, godot-cpp/modernise
My spreadsheet is updated, mainly for the msvc version, which is really annoying because I'm mostly a Linux guy. There are real problems that need to be addressed. https://docs.google.com/spreadsheets/d/1aUjib11P9CfcMK7P5WhdRIs80jMO9mGPK4RbMlSccCM/edit?usp=sharing
I just buttoned down the last few remaining items that were on my concerns list. I think I can now justify the tiny changes, like filenames matching the build config, compile definitions, and warnings as being slightly more correct than the master branch.
I'm very confident that this will improve the lives of everyone who chooses cmake, whether that is personal preference, or that their other dependencies require it.
Pretty sure this is done..
@dsnopek OK, so I am back from my holiday in Europe( which was great ) and I want to get back onto this.
I remember their being a request in the re-structure comments to update the docs/cmake.md with updated information. Is there anything else that is wanted from this?
Regarding the documentation, I've been trying to think(granted jetlag is still an issue) about the purpose of what is there and I think it is not coherant. Building the godot-cpp static library separately to an extension using cmake is not the cmake way.
I cant quite make an argument for the doc/cmake.md existence that would not have a more appropriate alternative.
@enetheru
OK, so I am back from my holiday in Europe( which was great ) and I want to get back onto this.
Awesome, thanks!
Building the godot-cpp static library separately to an extension using cmake is not the cmake way.
What would the cmake way be? Can we make our cmake configuration more closely match the cmake way?
I remember their being a request in the re-structure comments to update the docs/cmake.md with updated information. Is there anything else that is wanted from this?
I think this is the discussion you're thinking of.
The recommended cmake .
is problematic: it overwrites a file in the repo, and it doesn't seem like the cmake way. And when trying to use a separate build directory (which I think is more cmake-y), it doesn't seem to actually work.
So, we need to have a good approach to recommend that works, and update the docs to match.
Beyond that, it's been awhile since I reviewed/tested this PR, so I'll need to find some time to do that again.
@dsnopek As I understand the 'cmake way' is that you dont compile godot-cpp independently from your extension. So IMHO other than CI/CD any instructions provided should be on how to consume the library as a dependency of an extension project.
There is the godot-cpp-template project, which I am planning on submitting a cmake PR to.
If I can get the context and target audience straight in my head then I can write something that makes sense.
I'm leaning towards stating something like "Compiling godot-cpp independently of an extension project is for godot-cpp developers, package maintainers and CI/CD. Look to the godot-cpp-template for how to consume the godot-cpp library as part of your godot extension". Then I can provide simple instructions for compiling godot-cpp, plus elaborate on its currently supported features both as comparisons to scons, and cmake supported things.
OK I think I have rubber ducked this to the point I can continue.
OK so I had run into some trouble verifying the OSX build which forced me to re-think the approach I was taking. I was trying to do things in piecemeal, small single issue changes, but that was actually complicating the code when it came to making multi-platform work for more than two setups.
I eventually needed to incorporate the individual platforms as separate files to cater to the multi platform setup, which i was trying to put off, but could no longer. So thats the delay.
I have verified Linux, Darwin, Windows builds pass the test project. Since my current main machine is Windows it also builds Android and Web builds though I dont know how to test. And I have tested multiple other mingw toochains with tests reporting success.
In its current form it is almost a direct mirror of SCons so should be easy for a superficial comparison.
Cheers.
Thanks!
Overall, I love the direction this PR is going! I've skimmed through the code and it's mostly looking good, although, I haven't had a chance yet to compare the cmake config line-by-line with scons (which I do intend to try and do).
@dsnopek Cheers. keep in mind when doing the line-by-line is that I have purposely left out options that werent already present and not required for a platform. like thread support, and optimisation. but I can add them if required.
However, this is failing CI due to two minor spelling issues:
I really detest these spelling checks, but I understand why they need to be. I even have it setup on my build machine as a task I can run, but I forgot. These have been fixed.
And I've been unable to actually test this on my machine, because I get errors when doing:
mkdir build cd build cmake ../ -G "Ninja"
I get these errors:
<snip>
And it doesn't successfully generate the build files for me to use.
I can't reproduce, even when requiring cmake versions to the minimum. Can you let me know what your cmake version is? build platform etc?
I get different errors when I try to build the test project - doing:
cd test
<snip>
Am I doing something wrong?
CMake can't depend on directories above the project root like scons can, so the test project is part of the godot-cpp project as another target.
mkdir build
cd build
cmake ../
cmake --build . --target godot-cpp-test
I really detest these spelling checks, but I understand why they need to be. I even have it setup on my build machine as a task I can run, but I forgot. These have been fixed.
Thanks!
I can't reproduce, even when requiring cmake versions to the minimum. Can you let me know what your cmake version is? build platform etc?
I'm on Ubuntu 22.04, with cmake 3.22.1 (from the Ubuntu package).
Here's the full output.
UPDATE: I've tried doing some more testing. I thought that maybe the issue was caused by some files I have laying around from doing development in the same directory over a long period of time, so I downloaded a ZIP of your branch, and ran cmake
there, but I got the same issue. It's really weird, because the CI seems to also be using Ubuntu 22.04 and the Ubuntu cmake package, yet it works there...
cmake 3.22.1
I found the problem with my code:
$<LIST:GET,list,index,...>
Added in version 3.27.
I'll come up with a different way of doing that thing. Cheers for the help.
but I got the same issue
@dsnopek I really appreciate you testing and providing the logs.
I've replaced the offending generator expressions. I've also downloaded and tested with the project minimim version 3.13, I believe it should work now.
I did some amount of line-by-line comparison, but it's tricky, because so much is provided by CMake out-of-the-box. For example, there's practically nothing in
android.cmake
, but it's fine because all the features we have inandroid.py
for SCons appear to be built-in to CMake itself.
I can provide more information in that file in the way of documentation if you like? It cant hurt. I'll review the differences and see if I can't document how it works.
@enetheru:
I can provide more information in that file in the way of documentation if you like?
If you just mean putting a comment in android.cmake
saying something like "we don't have much here because CMake provides all the necessary Android options out-of-the-box", that'd probably be nice to have, but I don't think its essential.
EDIT: Ah, I see you already added it! It looks good to me :-)
scons uses builtin clang while my nix cmake uses g++13:
That's really interesting, I will try installing gcc on my mac mini and see if it has similar detection issues.
You may not have the same experience. I used nix
cmake specifically. Nix is very big on encapsulation, and if it used the system clang instead of shipping its own (g++ or clang++) i would have been very surprised.
You may not have the same experience. I used
nix
cmake specifically. Nix is very big on encapsulation, and if it used the system clang instead of shipping its own (g++ or clang++) i would have been very surprised.
yeah installing gcc through brew doesnt at all clobber the builtin compiler detection. I woud have to create a toolchain, or alter environment variables to change that. I'm thinking thta nix is sort of like how msys on windows cretes its own environment.
Thanks! :tada:
This PR builds on https://github.com/godotengine/godot-cpp/pull/1595 In this PR, I hope to consolidate all the existing code into one style so that we can start adding things that exist in the scons build but are missing from cmake that people have requested.
The primary changes are
Tests for Linux, Macos, and Windows are passing.
This style is cleaner and supports the multi-config generators like Visual Studio and Ninja-Multi-Config. Tho I find it slightly more annoying to debug personally, it is the better method.
I've been making sure these builds are Identical using this spreadsheet
Commit log