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: Handle GODOT_DEV_BUILD flag #1648

Merged
merged 1 commit into from
Dec 8, 2024
Merged

Conversation

enetheru
Copy link
Contributor

@enetheru enetheru commented Nov 23, 2024

When SCons has dev_build=yes enabled, there is a "dev" feature flag added to the compile artifact.
it enables debug_symbols unless otherwise specified
It changes the debug symbol generation
it sets the optimisation level to none
It sets DEV_ENABLED mutually exclusive to NDEBUG

Representing how dev_build and debug_symbols interact in CMake was harder than anticipated, I think I have struck a good balance.

debug_symbols will not be an option in CMake.
The generation of debug symbols is controlled by the build configuration. This is how it normally works in CMake and is the closest match to what the option means in SCons. For single configuration generators this is expressed by adding -DCMAKE_BUILD_TYPE=Debug or RelWithDebInfo which will generate debug symbols as expected by all software interacting with a cmake build system. And for multi-config generators adding --config=Debug or RelWithDebInfo to the build command. In both cases Debug is the default if not specified.

How this interacts with GODOT_DEV_BUILD
GODOT_DEV_BUILD does not enable debug genaration, but it will change the debug level
DEV_ENABLED will not be mutually exclusive with NDEBUG. A warning is presented if single config generator has both CMAKE_BUILD_TYPE=Release and GODOT_DEV_BUILD. For multi-config generators there is no way to warn for this exact mix, but they would both have to be set explicitly by the consumer.
Optimisation levels are not yet implemented.

I have updated documentation to descrive the deviations to SCons
Tidied existing debug flag handling that is now redundant
And tidied the helper variables and comments

Before I mark as ready for review I want to go over the documentation and comment strings at least once more. And perform a build option comparison on all three host platforms for all target platforms available to me.

Reminder, that Single-Config generators are Makefiles, Ninja, and Multi-Config generators are VisualStudio, XCode, 'Ninja Multi-Config'

SCons to CMake equivalency

debug_symbols=yes

  • SCons : scons target=template_debug debug_symbols=yes
  • CMake Single-Config gen : cmake ../ -DCMAKE_BUILD_TYPE=RelWithDebInfo && cmake --build .
  • CMake Multi-Config gen cmake ../ && cmake --build . --config RelWithDebInfo

dev_build=yes (automatically adds debug_symbols=yes)

  • SCons : scons target=template_debug dev_build=yes
  • CMake Single-Config gen : cmake ../ -DGODOT_DEV_BUILD=YES -DCMAKE_BUILD_TYPE=Debug && cmake --build .
  • CMake Multi-Config gen cmake ../ -DGODOT_DEV_BUILD=YES && cmake --build . --config Debug

dev_build=yes debug_symbols=no

  • SCons : scons target=template_debug dev_build=yes debug_symbols=no
  • CMake Single-Config gen : cmake ../ -DGODOT_DEV_BUILD=YES -DCMAKE_BUILD_TYPE=Release && cmake --build .
  • CMake Multi-Config gen cmake ../ -DGODOT_DEV_BUILD=YES && cmake --build . --config Release

Comparison Spreadsheet, sub sheets contain so far MSVC, Msys, and MacOS.

@enetheru enetheru requested a review from a team as a code owner November 23, 2024 10:52
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!

@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 Author

I'm sorry, dont merge this yet, I'm going to start submitting all my PR's as draft because I always find something to change.

In this case, the code is not defensive enough, if someone defines DEV in an outer scope, whatever that is will be injected in the name and we dont want that. So I'm going to ammend this for that case.

@enetheru enetheru marked this pull request as draft November 26, 2024 01:22
@enetheru enetheru marked this pull request as ready for review November 26, 2024 10:51
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 28, 2024

I just tested this locally and it doesn't seem to be working for me, although, I could be doing something wrong?

I built with:

mkdir cmake-build
cd cmake-build
cmake ../ -G "Ninja" -DGODOT_DEV_BUILD:BOOL=ON

# Just the godot-cpp static library:
cmake --build . -t template_debug --config Debug
ls bin
# SHOWS: libgodot-cpp.linux.template_debug.x86_64.a

# The test project.
cmake --build . -t godot-cpp-test --config Debug
ls ../test/project/bin
# SHOWS: libgdexample.linux.template_debug.x86_64.so

So, I'm not getting the .dev added

@enetheru enetheru marked this pull request as draft November 28, 2024 14:06
@enetheru
Copy link
Contributor Author

I'll do some re-testing and check back in later

@enetheru enetheru force-pushed the dev_tag branch 4 times, most recently from ea940de to 7a06374 Compare December 1, 2024 07:51
@enetheru enetheru changed the title CMake: add dev feature tag when GODOT_DEV_BUILD is enabled. CMake: Handle GODOT_DEV_BUILD flag correctly Dec 1, 2024
@enetheru enetheru force-pushed the dev_tag branch 2 times, most recently from dabcddb to e235a09 Compare December 3, 2024 02:26
@enetheru enetheru changed the title CMake: Handle GODOT_DEV_BUILD flag correctly CMake: Handle GODOT_DEV_BUILD flag Dec 4, 2024
@enetheru enetheru marked this pull request as ready for review December 4, 2024 22:49
@enetheru enetheru requested a review from dsnopek December 4, 2024 22:49
@dsnopek
Copy link
Collaborator

dsnopek commented Dec 6, 2024

Thanks! This is working for me when building the libgodot-cpp.linux.template_debug.dev.x86_64.a now.

However, should I expect this to work when building the test project? I'm still getting libgdexample.linux.template_debug.x86_64.so (without the .dev in there)

@enetheru
Copy link
Contributor Author

enetheru commented Dec 6, 2024

bgodot-cpp.linux.template_debug.dev.x86_64.

I figured out why, its a simple change, the ${DEV} flag is not in scope.
I will fix it soon.

.dev is added to output artifacts
Warn users for specifying dev_build and Release build config
Update documentation with deviations to SCons
Update debug_symbols handling, its rolled into build config
Cleanup helper variables and comments
@enetheru
Copy link
Contributor Author

enetheru commented Dec 6, 2024

However, should I expect this to work when building the test project?

fixed

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! It's working for me now :-)

@dsnopek dsnopek merged commit ce66e6b into godotengine:master Dec 8, 2024
11 checks passed
@enetheru enetheru deleted the dev_tag branch December 9, 2024 01:19
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.

2 participants