Skip to content

Conversation

@m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Apr 23, 2025

This addresses feedback from #105605 (comment).

As mentioned in #105605 (comment), I don't have a preference over which parameter to use, so looking for feedback.

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unified naming scheme is preferable

@syntaxerror247
Copy link
Member

syntaxerror247 commented Apr 23, 2025

I think gradle_do_not_strip is better here. It clearly indicates that it’s related to Gradle and aligns with the default behavior where Android strips symbols from .so files unless explicitly told not to. In contrast, separate_debug_symbols doesn’t make the Gradle context obvious and also flips the default behavior, now it avoids stripping by default.

I want to preserve the default gradle behaviour.

Copy link
Contributor

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

Personally I'd be fine either way (with this PR or keeping gradle_do_not_strip), but with maybe a slight preference for standardization among platforms.

@dsnopek
Copy link
Contributor

dsnopek commented Apr 28, 2025

The more I think about this, the more I think this PR is the way to go:

  • It's a little weird/unexpected that gradle_do_not_strip=yes prevents the generation of the separate debug symbols (ie there's no native-debug-symbols), because "not stripping" doesn't imply (at least to me) not generating the separate symbols. But if the option was separate_debug_symbols then it's very clear that yes generates separate symbols and no doesn't
  • PR [Web] Fix separate_debug_symbols=yes and debug experience #105808 was just posted implementing separate_debug_symbols for the web, and that's another platform setting expectations around this argument that Android would be different from

@m4gr3d m4gr3d force-pushed the switch_to_separate_debug_symbols branch from 58f3b0b to ccf6165 Compare April 28, 2025 15:33
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Apr 28, 2025

@dsnopek This is a good argument to keep consistency across the platforms, so I'm good for this PR to go as is!

I want to preserve the default gradle behaviour.

@syntaxerror247 The added logic should keep the default behavior:

if env["debug_symbols"] and not env["separate_debug_symbols"]:
        gradle_process += ["-PdoNotStrip=true"]

The addition of the doNotStrip=true flag is also guarded by the debug_symbols parameter so:

  • on regular build (e.g: scons platform=android generate_android_binaries=yes, gradle will strip the generated binaries, maintaining the default behavior
  • on build with debug_symbols enabled (e.g: scons platform=android generate_android_binaries=yes debug_symbols=yes), the doNotStrip=true parameter will be added, causing the generated binaries to include the debug symbols. I think we can argue this should be the default behavior when enabling debug_symbols as it allows engine developers the ability to quickly investigate issues without having to reach for an external debug symbols file

@Repiteo Repiteo merged commit ff33926 into godotengine:master Apr 28, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Apr 28, 2025

Thanks!

@m4gr3d m4gr3d deleted the switch_to_separate_debug_symbols branch April 29, 2025 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release discussion enhancement platform:android topic:buildsystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants