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: Revert to single cmake target #1733

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

Conversation

enetheru
Copy link
Collaborator

@enetheru enetheru commented Mar 7, 2025

re-mastered:

I've had this thought stuck in my head for a day or so, and a lingering doubt that I might be able to put to rest.

When I embarked on bringing godot-cpp cmake upto a more modern approach it was hard to grasp how it might fit into a standard cmake script. The way I initially thought about it was as three separate libraries, template_debug, template_release and editor.

This is why I made the solution create the three targets, so that users could configure and link to any of the targets they choose.

The latest thought is that this was a mistake on my part, there is only one library, and target is simply a configuration variable.
The output name is just an artifact of convenience and doesnt denote a separate library, but rather a separate configuration of the same library. I conflated terminology differences between the scons configuration and cmake and managed to turn godot target into cmake build target.

My mistaken choice has led to some confusion about the default target, and caused a departure from the synchonicity with SCons.

This PR is painful for me to make and I feel like it might be annoying or disruptive, I'm sorry for that.

Changes:

  • Removes the loop that creates multiple targets of godot-cpp.template_debug godot-cpp.template_release godot-cpp.editor. Now a single target is made, godot-cpp with the alias godot::cpp that defaults to template_debug
  • GODOTCPP_TARGET is a single value that can be one of the values: template_debug, template_release, editor.

In all other respects the cmake code is the same, and the work I have put into the whole thing remains intact and useful.

Configure and build commands would look like:

# default is template_debug with debug symbols
cmake -S <source_dir> -B <build_dir>
cmake --build <build_dir>

# Ninja, template_release in Release mode
cmake -S <source_dir> -B <build_dir> -DGODOTCPP_TARGET=template_release -DCMAKE_BUILD_TYPE=Release
cmake --build <build_dir>

msvc Ninja-Multi
cmake -S <source_dir> -B <build_dir> -DGODOTCPP_TARGET=template_release
cmake --build <build_dir> --config Release

@enetheru enetheru force-pushed the single_target_test branch from 1f0370b to 679b88b Compare March 9, 2025 01:41
@enetheru enetheru changed the title CMake: Single target test CMake: Revert to single cmake target Mar 9, 2025
@enetheru enetheru force-pushed the single_target_test branch from 9beadd2 to 4acb7ab Compare March 9, 2025 02:02
@enetheru
Copy link
Collaborator Author

There's a long discussion here that led me to this PR, in which both ghuser404 and torets approved this direction
#1730 (comment)

@Naros
Copy link
Contributor

Naros commented Mar 10, 2025

As we discussed on Orchestrator's discord, I agree with moving back to a singular cmake target.

In my experience, most IDE integrations for cmake are designed to be tightly coupled with a single profile that maps to a single CMAKE_BUILD_TYPE such as Debug, Release, RelWithDebInfo, etc. It's a bit unusual to have a single generator configuration that would build a debug, release, and editor build.

@enetheru enetheru marked this pull request as ready for review March 10, 2025 13:58
@enetheru enetheru requested review from a team as code owners March 10, 2025 13:58
@enetheru enetheru added the cmake label Mar 10, 2025
@enetheru enetheru force-pushed the single_target_test branch from 4acb7ab to 58ace59 Compare March 11, 2025 02:25
@enetheru
Copy link
Collaborator Author

Things were getting messy so i remastered this, its no longer based on anything else.

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.

Your explanation makes sense! I had a couple notes below, but I'm fine with going this direction in general.

It sounds like this also addresses the "default target" thing that a couple of folks have brought up, but in a slightly different way.

I attempted to test this locally, and it does seem to build fine!

(However, I'm having some unrelated issue locally where the unit tests are always crashing with stack smashing detected when using a build from CMake, but not from SCons. I'm not sure of the cause, but it doesn't appear to be connected with these changes - I get the problem even when reverting to older versions of godot-cpp that I'm pretty sure I tested in the past, so I'm suspecting something local to my system?)

@enetheru
Copy link
Collaborator Author

I'm suspecting something local to my system?)

Is your system something I can replicate? or remote into? I don't like having issues that go unexplained if I can help in some way.

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2025

I'm suspecting something local to my system?)

Is your system something I can replicate? or remote into? I don't like having issues that go unexplained if I can help in some way.

I made issue #1741 - I'm hoping someone else on Linux can test it and see if it's just me or not :-)

@enetheru enetheru force-pushed the single_target_test branch from 58ace59 to 98d0777 Compare March 11, 2025 22:19
@enetheru
Copy link
Collaborator Author

@dsnopek Updated with requested changes.

Add GODOTCPP_TARGET configuration option
Remove loop to generate the godot-cpp.<target> CMake Targets

Rename test bindings target
Update documentation
@enetheru enetheru force-pushed the single_target_test branch from 98d0777 to 89abe15 Compare March 11, 2025 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants