[gn] fix --stripped being disabled for all targets#181984
[gn] fix --stripped being disabled for all targets#181984auto-submit[bot] merged 1 commit intoflutter:masterfrom
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request correctly fixes a bug where the --stripped flag was not using its intended default behavior. By changing the default value of the argparse argument to None, the script can now differentiate between the flag being absent and being explicitly set, allowing for the correct default logic to be applied. The updated help string also accurately reflects this change. I have one minor suggestion to improve code formatting by wrapping a long line.
release build got much bigger since flutter 3.32 (for debug it went from 83MB to 385MB) This is apparently just because the lib is no longer stripped since [1] This looks like a bug since we don't target android (tentative fix in [2]), but until that lands just set --stripped manually. Even if there is no C symbol dart stacktraces are available so most people don't need these. Link: flutter/flutter#161546 [1] Link: flutter/flutter#181984 [2]
flutter#161546 changed the stripped option with the intent to be true for android, and false for all other targets, but the check never worked as intended because parser store_true action default value is False so the final else was never used. This fixes the build size increase that was noticed on flutter 3.32 for linux targets
|
@reidbaker can you please take a look here? It seems like it would be nice if we had unstripped for debug builds to make it easier to work with things like Android Studio. I'm not sure of all the ramifications though. |
Related #181275. |
|
I've read most related changes around this and as far as I can tell they're mostly about android targets, which will keep its default non-stripped with this patch, but honestly I don't know enough to leaving the decision to you. Ideally if we enable stripping by default it'd be great to have something like #181377 (move debug symbols out to a separate file instead of huge .so that are copied around), but for now I'd be happy so long as the help is correct and the code doesn't have an obviously wrong "if option is neither true nor false" else close, so just let me know if you prefer to unconditionally not strip and I'll update the PR. |
You are correct, the change was only intended to impact android targets. Thank you for reporting the issue and raising the PR. I would guess the original issue which you link was closed by mistake, assuming that the topic was the change to android targets. As @gaaclarke mentions the unstripped build could be helpful in the debug case, but it was certainly not the intent of my PR to change the non-android targets, so I think we should land this independently of that discussion |
gmackall
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/181984, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
reidbaker
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
flutter#161546 changed the stripped option with the intent to be true for android, and false for all other targets, but the check never worked as intended because parser store_true action default value is False so the final else was never used. This fixes the build size increase that was noticed on flutter 3.32 for linux targets Fixes flutter#181983 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing.
4 participants
#161546 changed the stripped option with the intent to be true for android, and false for all other targets, but the check never worked as intended because parser store_true action default value is False so the final else was never used.
This fixes the build size increase that was noticed on flutter 3.32 for linux targets
Fixes #181983
Pre-launch Checklist
///).