Skip to content

Conversation

@enetheru
Copy link
Collaborator

This PR resolves CMake name clashes #1657, Partially resolves optionally disable targets #1650, and tidies IDE usage for VS and XCode by grouping targets under a subfolder.

  • Rename CMake internal targets from to godot-cpp., Aliases will remain godot-cpp::
  • Simplify _generate cmake function signature, as TARGET_ALIAS and TARGET_NAME are still in scope and do not need to be passed.
  • Add GODOT_ENABLE_TESTING option default OFF, and guard add_subdirectory( test ) by default
  • Add global property USE_FOLDERS ON and target property FOLDER "godot-cpp" to group targets in VS and XCode
@Torets
Copy link

Torets commented Nov 27, 2024

The thing GODOT_ENABLE_TESTING somewhat clashes with my PR #1650, I also guard add_subdirectory( test ), but with GODOT_CPP_USE_TEST that is ON by default. I failed to mention it in PR description, sorry.
I'll remove it from my PR to avoid future merging conflicts.

@enetheru
Copy link
Collaborator Author

The thing GODOT_ENABLE_TESTING somewhat clashes with my PR #1650

Indeed it does clash, perhaps it needs its own PR, if either of our PR's get merged, the other can rebase and fix. I thought about splitting this into three parts, but I dont know how fine grained PR's should be.

Torets added a commit to Torets/godot-cpp-optional-targetss that referenced this pull request Nov 27, 2024
@Torets
Copy link

Torets commented Nov 27, 2024

I've reverted my changes for now. yours seem to better in terms of naming, description of PR and placement of option.

@enetheru enetheru force-pushed the name_clash branch 3 times, most recently from 9d74b0e to db37472 Compare November 28, 2024 15:19
@enetheru enetheru force-pushed the name_clash branch 2 times, most recently from 1541901 to c6af953 Compare December 5, 2024 00:01
@enetheru
Copy link
Collaborator Author

enetheru commented Dec 5, 2024

rebased onto #1651

@enetheru enetheru marked this pull request as ready for review December 5, 2024 00:02
@enetheru enetheru requested review from a team as code owners December 5, 2024 00:02
@dsnopek
Copy link
Collaborator

dsnopek commented Dec 6, 2024

Thanks! This makes sense in general: users of godot-cpp don't want to end up with a target for godot-cpp internal tests. Seems to be failing CI at the moment, though

@enetheru
Copy link
Collaborator Author

enetheru commented Dec 6, 2024

Thanks! This makes sense in general: users of godot-cpp don't want to end up with a target for godot-cpp internal tests. Seems to be failing CI at the moment, though

Still getting used to re-basing I think something was lost. I will fix it soon.

@enetheru enetheru force-pushed the name_clash branch 3 times, most recently from 89d9038 to 611c951 Compare December 9, 2024 01:44
@dsnopek
Copy link
Collaborator

dsnopek commented Dec 9, 2024

Thanks!

Unfortunately, looks like this needs another rebase for conflicts.

Also, I think it would be nice to update the cmake.rst docs with info about how to build the test project with this new option.

@enetheru enetheru force-pushed the name_clash branch 2 times, most recently from e497679 to 78d71c2 Compare December 10, 2024 00:43
@enetheru
Copy link
Collaborator Author

Also, I think it would be nice to update the cmake.rst docs with info about how to build the test project with this new option.

So I went and updated the documentation as asked. While doing so I really hated that I previously introduced the TEST_TARGET option to control which target the test would link against. So I made it go away, now all the variants are generated with name godot-cpp.test.<target>. I had to modify the CI and documentation anyway, so it seemed like a time to change it.

@enetheru enetheru force-pushed the name_clash branch 3 times, most recently from 28ed5df to a282bff Compare December 11, 2024 12:21
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.

Thanks!

Simplify <platform>_generate cmake function signature, as TARGET_ALIAS and TARGET_NAME are still in scope and do not need to be passed. Add USE_FOLDERS, and FOLDER properties to group targets in VS and XCode Guard test target with GODOT_ENABLE_TESTING Generate all three variants in the form godot-cpp.test.<target> to remove the -DTEST_TARGET option clutter. Update the docs/cmake.rst with information about the testing target Update the Github CI
@dsnopek dsnopek merged commit 012b8ff into godotengine:master Jan 10, 2025
11 checks passed
@dsnopek dsnopek added this to the 4.x milestone Jan 10, 2025
@dsnopek dsnopek added the cmake label Jan 10, 2025
@enetheru enetheru deleted the name_clash branch January 11, 2025 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants