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.

Sign up for GitHub

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

Jump to bottom

CMake: Alleviate target name clashes, visibility, and grouping. #1658

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

CMake: Alleviate target name clashes, visibility, and grouping. #1658

wants to merge 1 commit into from

Conversation

enetheru
Copy link
Contributor

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 •
edited
Loading

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
Contributor 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
restored changed to CMakeLists.txt to avoid conflicts with PR godoten…
45a71a7 
…gine#1658
@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
Contributor 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
Contributor 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
Contributor 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.

dsnopek
dsnopek reviewed Dec 10, 2024
View reviewed changes
doc/cmake.rst Outdated Show resolved Hide resolved
@enetheru enetheru force-pushed the name_clash branch 2 times, most recently from dd32f97 to 28ed5df Compare December 11, 2024 00:12
@enetheru
Alleviate CMake target name clashes, visibility, and grouping.
a282bff 
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 approved these changes Dec 11, 2024
View reviewed changes
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!

@Torets Torets mentioned this pull request Dec 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfranke aaronfranke aaronfranke approved these changes

@dsnopek dsnopek dsnopek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@enetheru @Torets @dsnopek @aaronfranke