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: Remove hardcoded warnings list and forcing -Werror on library builds #1324

Merged

Conversation

akien-mga
Copy link
Member

The CMake buildsystem should be completely reviewed to properly match what is done by SCons, instead of making its own arbitrary decisions on how godot-cpp should be compiled.

Currently the SCons setup doesn't include warning options, so CMake shouldn't either. Options similar to upstream Godot's SCons setup could be added, and then replicated for CMake.


This kind of issue is exactly why I'm against having multiple buildsystems. It's very difficult to keep them in sync, especially since officially we only commit to maintaining SCons, yet many users reach straight for CMake as more familiar to them (or usually more familiar to their IDE...).

If we really want to support CMake (which can make sense for something used to interface with thirdparty codebases, often using CMake), then it probably needs to be redone from scratch by someone who understands and cares about the SCons buildsystem, and can strive to keep them in sync. That won't be me :)

…builds

The CMake buildsystem should be completely reviewed to properly match
what is done by SCons, instead of making its own arbitrary decisions on
how godot-cpp should be compiled.

Currently the SCons setup doesn't include warning options, so CMake
shouldn't either. Options similar to upstream Godot's SCons setup could
be added, and then replicated for CMake.
@akien-mga akien-mga added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup cherrypick:4.1 cherrypick:4.2 labels Dec 1, 2023
@akien-mga akien-mga added this to the 4.x milestone Dec 1, 2023
@akien-mga akien-mga requested a review from a team as a code owner December 1, 2023 09:50
@akien-mga
Copy link
Member Author

I see that #1007 aims at adding warning options to SCons - so once it's finalized and merged, this can be revisited to add the same warnings to CMake.

I insist that if we support multiple buildsystems, we should strive to make them produce the exact same output. So they need to take the exact same input.

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 1, 2023

This kind of issue is exactly why I'm against having multiple buildsystems. It's very difficult to keep them in sync, especially since officially we only commit to maintaining SCons, yet many users reach straight for CMake as more familiar to them (or usually more familiar to their IDE...).

If we really want to support CMake (which can make sense for something used to interface with thirdparty codebases, often using CMake), then it probably needs to be redone from scratch by someone who understands and cares about the SCons buildsystem, and can strive to keep them in sync. That won't be me :)

I agree that this is a problem.

Right now, the majority of open PRs on godot-cpp are build system related. And a sizeable percentage are cmake related (they aren't all tagged, so I can't easily get a count, but it could even be a majority). But, personally, I don't just know cmake, so beyond the simplest changes, I can't personally review them either.

We really need someone on the GDExtension team who understands and cares about cmake, in addition to understanding how the scons build system works so we can be sure that both are working in the same way with the same overall philosophy.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Insofar as I understand cmake, these changes make sense.

Locally, I tested building godot-cpp on it's own and the tests using cmake with the make backend - it seemed to work fine!

@akien-mga akien-mga merged commit 39ca745 into godotengine:master Dec 7, 2023
12 checks passed
@akien-mga akien-mga deleted the cmake-remove-hardcoded-warnings branch December 7, 2023 07:57
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Dec 12, 2023
@akien-mga
Copy link
Member Author

Cherry-picked for 4.2.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

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 topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants