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

SCons: Improve debug symbol logic & cleanup #91669

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

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented May 7, 2024

Depends on #91847

This PR improves the debugsymbol logic overall in a number of ways:

  • Ensures that debugsymbol files will be cleaned up when passing --clean to SCons. Logic to support something similar already existed in the Windows SCSub via arrange_program_clean, but that's now been migrated to methods.py & refactored as a universal environment function.
  • The original debugsymbol functions in platform_*_builders.py have been migrated to their respective SCSub, being cleaned up in the process. Consequently, the original files have been removed, as they only had that single function & the files themselves became redundant once subprocess dependencies were removed1.
  • Windows now correctly checks for both debug_symbols and separate_debug_symbols before attempting separation; previously, it only checked for the former. EDIT: Working as intended.
  • Windows now checks if objcopy and strip exist in detect.py instead of SCSub, and throws a proper error if they're missing. This allows pulling their MinGW paths only once & will alert the user to issues prior to building instead of at the very last second.
  • Windows now assigns .debugsymbol extension to program stem instead of the existing name (.exe.debugsymbols.debugsymbols). This matches the naming scheme of the other Windows program files that require explicit cleanup (.ilk, .exp, .pdb, .lib).

Footnotes

  1. SCons: Remove run_in_subprocess & subprocess_main dependencies #89393

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Windows now correctly checks for both debug_symbols and separate_debug_symbols before attempting separation; previously, it only checked for the former.

This was intentional, MinGW build should check for executable size and force debug symbols if it's too large (see #79875), but seems like the logic is already broken by #85319, the following two lines were likely accidentally removed during rebase:

    # Force separate debug symbols if executable size is larger than 1.9 GB.
    if env["separate_debug_symbols"] or os.stat(target[0]).st_size >= 2040109465:

@Repiteo Repiteo force-pushed the scons/better-debugsymbol-funcs branch 2 times, most recently from 4d5cc34 to cb42781 Compare May 8, 2024 19:29
@Repiteo

This comment was marked as outdated.

@bruvzg

This comment was marked as outdated.

@fire fire changed the title SCons: Improve debugsymbol logic & cleanup SCons: Improve debug symbol logic & cleanup May 8, 2024
@Repiteo
Copy link
Contributor Author

Repiteo commented May 8, 2024

It seems to still only look for prefixed executables, so probably not fixed.

It only looks for a prefixed executable when checking if Windows is a supported platform, ensuring no false positives. If Windows is supported & selected for building, the mingw_prefix bin folder is prepended to the SCons Environment's path

@bruvzg
Copy link
Member

bruvzg commented May 8, 2024

If Windows is supported & selected for building, the mingw_prefix bin folder is prepended to the SCons Environment's path

I have missed that it's checking for env to set prefix, not sure if it is the best way for code readability, but should work indeed.

@akien-mga
Copy link
Member

Expanded this PR slightly to include a fix for #91710, as it was already dealing with get_mingw_tool & general Windows detect.py cleanup.

I'd prefer it on a standalone PR to fix the regression only. Build system changes have a high risk of breaking specific configs so I prefer we first fix the known issues, before adding potential new ones by fast tracking an enhancement PR.

@Repiteo Repiteo force-pushed the scons/better-debugsymbol-funcs branch from cb42781 to 800bca3 Compare May 8, 2024 20:51
@Repiteo
Copy link
Contributor Author

Repiteo commented May 8, 2024

That's a really good point. Removed the fix from this PR & migrated the code to #91734 instead.

@Repiteo Repiteo force-pushed the scons/better-debugsymbol-funcs branch 3 times, most recently from b86dee4 to aa1ad48 Compare May 11, 2024 17:24
• Relocate `arrange_program_clean` from Windows `SCsub` to `methods.py`; refactor as an universal environment function
• Remove `platform_*_builders.py` files, logic relocated and refactored in respective SCsub files
@Repiteo Repiteo force-pushed the scons/better-debugsymbol-funcs branch from aa1ad48 to 1299334 Compare May 12, 2024 15:31
@Repiteo Repiteo marked this pull request as draft May 12, 2024 15:32
@Repiteo
Copy link
Contributor Author

Repiteo commented May 12, 2024

Changing to a draft now that this is a dependency of #91847.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants