-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
[flutter_tools] support --split-debug-info option in android builds #49650
[flutter_tools] support --split-debug-info option in android builds #49650
Conversation
The actual "flutter symbolicate" command is a work in progress here: #49465 |
Adding @zanderso for initial thoughts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write here or in the symbolize PR a summary of what the commands in a typical workflow would look like for this?
final String archName = darwinArch == null | ||
? getNameForTargetPlatform(platform) | ||
: getNameForDarwinArch(darwinArch); | ||
final String debugFilename = 'app.$archName.debug'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files should be added to the .gitignore template
@@ -106,6 +106,7 @@ class FlutterOptions { | |||
static const String kEnableExperiment = 'enable-experiment'; | |||
static const String kFileSystemRoot = 'filesystem-root'; | |||
static const String kFileSystemScheme = 'filesystem-scheme'; | |||
static const String kDebuggingInfoOption = 'save-debugging-info'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag not only causes the debugging info to be saved off to the side, but also causes the VM to print debugging info in a different format, so I think we should consider a more descriptive flag name. Maybe --compress-debug-info
, or --split-debug-info
, or --separate-debug-info
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like split because it is easier to spell than separerate separate
@@ -363,6 +364,12 @@ abstract class FlutterCommand extends Command<void> { | |||
negatable: false, | |||
hide: !verboseHelp, | |||
help: 'Build a JIT release version of your app${defaultToRelease ? ' (default mode)' : ''}.'); | |||
argParser.addOption(FlutterOptions.kDebuggingInfoOption, | |||
help: 'Save a debug file used to symbolicate crashes from release mode applications. When provided ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indicate somehow that this is a path to a directory.
Along with the workflow, we'll probably need some official docs on flutter.dev and possibly more |
@@ -33,5 +33,17 @@ | |||
# Web related | |||
lib/generated_plugin_registrant.dart | |||
|
|||
# Symbolication related | |||
app.android-arm.debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "app.-.debug" work so that we don't have to update this?
final String archName = darwinArch == null || platform == TargetPlatform.darwin_x64 | ||
? getNameForTargetPlatform(platform) | ||
: 'ios-${getNameForDarwinArch(darwinArch)}'; | ||
final String debugFilename = 'app.$archName.debug'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if an app with different entrypoints is built out of the same project? Maybe these should be named like "$entrypoint-symbols.$archName.debug".
Also wdyt about an extension of "symbols" instead of "debug", so it would be entrypoint.arch.symbols instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like symbols as an extension better, since it will match the symbolize command.
Beyond the entrypoint, there is the current dart+flutter version, pubspec version, in the future dart-defines. Rather than trying to capture all of it, I think we should document and explain that each file is 1-1 with a particular release.
@@ -363,6 +364,15 @@ abstract class FlutterCommand extends Command<void> { | |||
negatable: false, | |||
hide: !verboseHelp, | |||
help: 'Build a JIT release version of your app${defaultToRelease ? ' (default mode)' : ''}.'); | |||
argParser.addOption(FlutterOptions.kSplitDebugInfoOption, | |||
help: 'A directory where a debug files for symbolication can be safely stored. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternate wordsmithing:
In a release build, this flag reduces application size by storing Dart program symbols in a separate file on the host rather than in the application. The value of the flag should be a directory where program symbol files can be stored for later use. These symbol files contain the information needed to symbolize Dart stack traces. For an app built with this flag, the 'flutter symbolize' command with the right program symbol file are required to obtain a human readable stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The length of our explanations here suggests a website article is in order too
app.fuchsia-arm64.debug | ||
app.fuchsia-x64.debug | ||
app.ios-armv7.debug | ||
app.ios-arm64.debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline
This is ready for another review pass - TBD if it should actually be landed before the flutter symbolize command is ready though |
// the architecture, since a single build command may produce | ||
// multiple debug files. | ||
final String archName = darwinArch == null || platform == TargetPlatform.darwin_x64 | ||
? getNameForTargetPlatform(platform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider modifying getNameForTargetPlatform()
to take an optional named DarwinArch
parameter.
This logic could also use a test since this file will be the glue between this flag and flutter symbolize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here, plus a simple unit test.
The gen_snapshot tests also assert the addition of the correct string arg
@@ -55,6 +56,11 @@ class BuildInfo { | |||
/// On Xcode builds it is used as CFBundleShortVersionString, | |||
final String buildName; | |||
|
|||
/// A optional directory path to save debugging information from dwarf stack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A optional -> An optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -363,6 +364,16 @@ abstract class FlutterCommand extends Command<void> { | |||
negatable: false, | |||
hide: !verboseHelp, | |||
help: 'Build a JIT release version of your app${defaultToRelease ? ' (default mode)' : ''}.'); | |||
argParser.addOption(FlutterOptions.kSplitDebugInfoOption, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably reserve addBuildModeFlags()
for the top-level build modes that map to the fields of BuildMode
. This flag can go in its own add...Flag()
method, or another existing one if there's one that's appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
'symbol files can be stored for later use. These symbol files contain ' | ||
'the information needed to symbolize Dart stack traces. For an app built ' | ||
'with this flag, the \'flutter symbolize\' command with the right program ' | ||
'symbol file is required to obtain a human readable stack trace', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can land this before flutter symbolize
has landed if the github issue for flutter symbolize
is noted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm w/ small suggestion
@@ -390,7 +396,10 @@ DarwinArch getIOSArchForName(String arch) { | |||
return null; | |||
} | |||
|
|||
String getNameForTargetPlatform(TargetPlatform platform) { | |||
String getNameForTargetPlatform(TargetPlatform platform, {DarwinArch darwinArch}) { | |||
if (darwinArch != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this to a nested switch under the TargetPlatform.ios
case. I think that's the only case where it means anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Work towards #48726
Allows further size reduction in AOT snapshots for android builds. Adds an additional argument '--save-debugging-info'. This is expected to point at a directory where the tool may write one or more debug files created during the build process. The act of specifying this argument enables dwarf stack traces.
I opted for this design over a general optimization level, because we need a user provided location for debug files. Placing them in build output folders isn't safe, since we have such a high usage of
flutter clean
.This doesn't fully wire up support for iOS, since we still need to resolve why we're using xcode to strip symbols instead of gen_snapshot (see #49628 )
Example Workflow:
Developer is ready to release an APK for distribution on the Play Store.
They create a directory for the specific version of the app to be released. Example:
symbols/v1.2.3-final-final
They run
flutter build apk --release --split-debug-info=symbols/v1.2.3-final-final/
They otherwise upload the APK as normal. The symbol files are either stored locally, or commited to some internal symbol database.
They receive a crash report from version v1.2.3-final-final on an arm64 ABI.
They retrieve the corresponding symbols file generated in step two.
They run
flutter symbolize --symbols=v1.2.3-final-final/app.android-arm64.symbols < stack.txt
What if the symbols file is lost or destroyed? In theory it could be regenerated by being on the exact same version of flutter with the exact same dart code