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

Modernise existing cmake options #1598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

enetheru
Copy link
Contributor

@enetheru enetheru commented Sep 19, 2024

This PR builds on #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

  • Adding all the {platform}.cmake files
  • The moving of all conditional target options/definitions to generator expressions.
  • And specifying the source files explicitly.

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

- Added to .gitignore CMakeUserPresets.json

### Configuration:
- Changed python command to use single quotes to make build output log more legible.
- Added GODOT_DEV_BUILD to allow differentiation of debug or Release builds.
- Added find logic for macos Cocoa library

### godot-cpp Changes
- godot-cpp-test is changed to be incorporated into the cmake build as a target.
- Duplicated godot-cpp target into [template_release, template_debug, editor]
- Created cmake/sources.cmake to collect all the pre-existing source files, because globing is a source of bugs.
- Created {platform}.cmake files mirroring the style of the SCons build.

CMake has a feature called generator expressions for its configuration variables that are evaluated at build time. This allows multi-configuration build systems to properly evaulate options. for msvc, xcode and nijna multi-config.

- Moved configuration options to generator expressions with the notable exclusion of OSX_ARCHITECTURES.
- Remove CMAKE_BUILD_TYPE from msvc CI target as Multi-Config generators ignore it

### godot-cpp-test Changes
- Removed majority of the cmake code, now that the godot-cpp project is setup, the majority of the flags will be propagated as transient dependencies
- Marked with EXCLUDE_FROM_ALL so that it isn't built as part of the 'all' target
- Updated ci to build the godot-cpp-test target from the root directory using cmake
- Tests passing for Windows, Linux, and Macos builds.

### Documentation
Updated with new information
Added Emscripten example
Added Android example


@enetheru
Copy link
Contributor Author

Looks like I have to fix up the test case

@enetheru
Copy link
Contributor Author

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.

@enetheru enetheru force-pushed the modernise branch 2 times, most recently from 461fa63 to c51489b Compare September 19, 2024 11:50
@enetheru enetheru marked this pull request as ready for review September 19, 2024 12:08
@enetheru enetheru requested review from a team as code owners September 19, 2024 12:08
@dsnopek dsnopek added this to the 4.x milestone Sep 19, 2024
@enetheru
Copy link
Contributor Author

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.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Overall the organization in this PR looks great. There's a lot missing still but as you stated that is expected for this first PR. I like how it lays out a plan for what's to come. When we get to implementing features such as architecture handling in a future PR I have a branch here, which enetheru already knows about, just linking for easy access.

I tried testing this PR. I was able to build godot-cpp by itself, but I was not able to get the test project to build. I tried running cmake --build . --verbose -j $(nproc) -t godot-cpp-test (copied from the CI script in this PR) on macOS and it fails silently, no compiled files showed up in the bin folder. Although I wasn't able to get the test project working on master either, with a lot of errors being printed on master.

I also tried compiling Godot Orchestrator with this PR, which is a big Godot project using CMake. I had to make two expected changes to Orchestrator to make it compile: replacing INCLUDE(GodotCompilerWarnings) with INCLUDE(common_compiler_flags) (it's fine to have to make changes, since we don't guarantee API stability with the CMake files) and changing the Godot minor version to 3 to work around an Orchestrator bug. However, it then failed to compile with the error c++: error: unsupported option '-static-libgcc'. I've already opened a bug report for this here #1522 but it's strange that the bug didn't show up with Orchestrator with master, and doesn't show up in godot-cpp by itself either with master or this PR. Anyway, worth looking into if we can fix #1522 - AFAICT we shouldn't ask clang to be static libgcc since it's not gcc (or, if there is an obscure reason why it is needed, then there needs to be a comment, and we need to fix whatever is causing compilation to fail). However, just removing this line doesn't fix it, it will then complain about -R being unrecognized. Anyway, if fixing #1522 is not in-scope then it can be fixed in a follow-up PR.

doc/cmake.md Outdated
Generate the buildfiles in a sub directory to not clutter the root directory with build files:

```shell
mkdir build && cd build && cmake -G "Unix Makefiles" .. && cmake --build .
Copy link
Member

@aaronfranke aaronfranke Sep 20, 2024

Choose a reason for hiding this comment

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

It would be nice if we could standardize some folder name (I'd vote for cmake_build or similar) and use it in the CI scripts. This way we can put it in the .gitignore file, and also encourage using a subfolder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't really touched that document except to convert it to markdown. its still 1:1 with the old block of comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronfranke I have now updated the document extensively, please have a look and let me know what you think.

@enetheru enetheru marked this pull request as draft September 20, 2024 08:15
@enetheru
Copy link
Contributor Author

I found some issues with my hasty conversion of the test project.

@enetheru
Copy link
Contributor Author

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.

@enetheru
Copy link
Contributor Author

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.

@enetheru
Copy link
Contributor Author

@enetheru
Copy link
Contributor Author

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

@enetheru
Copy link
Contributor Author

@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

@enetheru
Copy link
Contributor Author

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

@enetheru enetheru marked this pull request as ready for review September 23, 2024 03:03
@enetheru
Copy link
Contributor Author

enetheru commented Sep 23, 2024

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.

@enetheru
Copy link
Contributor Author

Pretty sure this is done..

@enetheru
Copy link
Contributor Author

@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.

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 21, 2024

@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.

@enetheru
Copy link
Contributor Author

@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.

- Added to .gitignore CMakeUserPresets.json

### Configuration:
- Changed python command to use single quotes to make build output log more legible.
- Added GODOT_DEV_BUILD to allow differentiation of debug or Release builds.
- Added find logic for macos Cocoa library

### godot-cpp Changes
- godot-cpp-test is changed to be incorporated into the cmake build as a target.
- Duplicated godot-cpp target into [template_release, template_debug, editor]
- Created cmake/sources.cmake to collect all the pre-existing source files, because globing is a source of bugs.
- Created {platform}.cmake files mirroring the style of the SCons build.

CMake has a feature called generator expressions for its configuration variables that are evaluated at build time. This allows multi-configuration build systems to properly evaulate options. for msvc, xcode and nijna multi-config.

- Moved configuration options to generator expressions with the notable exclusion of OSX_ARCHITECTURES.
- Remove CMAKE_BUILD_TYPE from msvc CI target as Multi-Config generators ignore it

### godot-cpp-test Changes
- Removed majority of the cmake code, now that the godot-cpp project is setup, the majority of the flags will be propagated as transient dependencies
- Marked with EXCLUDE_FROM_ALL so that it isn't built as part of the 'all' target
- Updated ci to build the godot-cpp-test target from the root directory using cmake
- Tests passing for Windows, Linux, and Macos builds.

### Documentation
Updated with new information
Added Emscripten example
Added Android example
@enetheru
Copy link
Contributor Author

enetheru commented Nov 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants