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

Added options to enable/disable targets to be added to a cmake build. #1650

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Torets
Copy link

@Torets Torets commented Nov 23, 2024

In CMake build system all of the targets were added to build (solution for Visual Studio), which might case confusion
and unnecessary additional build time.
I have added following options to CMake:

  • GODOT_ADD_TARGET_TEMPLATE_RELEASE - for target template_release
  • GODOT_ADD_TARGET_TEMPLATE_DEBUG - for target template_debug
  • GODOT_ADD_TARGET_EDITOR - for target editor
    These options are all ON by default to preserve the same default behavior as prior their addition. By turning options OFF correlating targets will be excluded from build. If all options are OFF CMake will rise an error during configuration phase.

Additionally to make sure that 'Test' target dependency stays valid, the following logic was added. If dependency gets turned off by corresponding GODOT_ADD_TARGET_*, then the dependency is switched to next target in following order template_debug -> template_release ->editor.

Also separate flag GODOT_CPP_USE_TEST with default value ON was added to have an ability to turn off testing as well.

These changes can be further supported in Fix break prior modernization PR by adding ${GODOT_DEFAULT_TARGET} ALIAS instead of hardcoded template_debug after both PRs are merged. Even without the change default behavior of CMake build should stay the same as it was before

To support this change and prevent erroneous cmake variable values, logic around choosing test dependency was also modified.
@Torets Torets requested a review from a team as a code owner November 23, 2024 18:28
@enetheru
Copy link
Contributor

This concept arose in conversation between @Torets and I while discussing the changes in master.

Being able to disable/enable the targets gives a best of both worlds scenario when dealing with CMake and IDE's that have cluttered target lists, and build everything like Visual Studio does. But does not prevent building everything all at once if desired.

@dsnopek dsnopek added cmake topic:buildsystem Related to the buildsystem or CI setup labels Nov 25, 2024
@dsnopek dsnopek added this to the 4.x milestone Nov 25, 2024
@enetheru
Copy link
Contributor

Hey @Torets I was doing some testing, and setup a new project which fetched godot-cpp. Visual studio didnt build anything at all when triggering the all build. Can you re-check with a fresh source tree that the build-all in visual studio does/does not infact try to build all targets for you? I had to consume a target, or remove EXLCUDE_FROM_ALL to get it to want to build the library when clicking the build all, i htink f6, or from the build menu.

@Torets
Copy link
Author

Torets commented Nov 28, 2024

@enetheru
It does for me. Though I've got version from my clone. If I check Project Dependencies, here is what I have for ALL_BUILD:
изображение

@Torets
Copy link
Author

Torets commented Nov 28, 2024

The only EXCLUDE_FROM_ALL I have in my version is in CMakeLists.txt from my folder. And regardless of having it for the rest of projects or no and having folders for them (which is always good), I think having an option to remove projects from *.sln is needed QoL feature

@enetheru
Copy link
Contributor

The only EXCLUDE_FROM_ALL I have in my version is in CMakeLists.txt from my folder

what? there should be an ... crap
do you see this add_library( ${TARGET_NAME} STATIC ${EXCLUDE} ) somwhere around godot-cpp/cmake/godotcpp.cmake:~243?
that ${EXLCUDE} is supposed to read EXCLUDE_FROM_ALL and was from a previous attempt at making the default template_debug, but it just got in the way.

add_library cmake docs

think having an option to remove projects from *.sln is needed QoL feature

I agree with you, I think it is a good idea, I just had conflicting information I needed to clean up in my mind.

@Torets
Copy link
Author

Torets commented Nov 28, 2024

yeah, that EXCLUDE is always empty right now from what I see. I can assume it could get in a way of passing test or some other default behavior. If not, may then I return it in your PR?

@Torets
Copy link
Author

Torets commented Dec 13, 2024

Since this PR will also have conflicts with PR CMake: Alleviate target name clashes, visibility, and grouping. I will solve conflicts after that one will be successfully merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants