Skip to content

Commit df114fb

Browse files
gmackallGray Mackall
andauthored
Remove default for stripped option in engine/src/flutter/tools/gn, don't strip by default on android (flutter#161546)
Remake of flutter/engine#52852. Makes it so that `stripped` defaults to false for android in `gn` arguments, i.e. we don't strip the Android engine builds. AGP does this by default when the NDK is installed (and we download it automatically now after flutter#159756). In testing, the step that AGP does to strip symbols adds ~1-2 seconds to the build. Adds a tool verification for release app bundle builds that checks to make sure we have stripped the debug symbols and placed them in the [`BUNDLE-METADATA` directory](https://developer.android.com/guide/app-bundle/app-bundle-format). The check is done by invoking `apkanalyzer`, which takes ~`0.5` seconds, which is why the check is only enabled for release builds. BEFORE LANDING: I need to follow up on flutter/engine#50443 (comment), to ensure we start stripping symbols internally as well. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <[email protected]>
1 parent 8e2a6fc commit df114fb

File tree

3 files changed

+506
-2
lines changed

3 files changed

+506
-2
lines changed

engine/src/flutter/tools/gn

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,12 @@ def to_gn_args(args):
666666
if args.build_embedder_examples is not None:
667667
gn_args['build_embedder_examples'] = args.build_embedder_examples
668668

669-
gn_args['stripped_symbols'] = args.stripped
669+
if args.stripped is True:
670+
gn_args['stripped_symbols'] = True
671+
elif args.stripped is False:
672+
gn_args['stripped_symbols'] = False
673+
else:
674+
gn_args['stripped_symbols'] = args.target_os != 'android'
670675

671676
if args.msan:
672677
gn_args['is_msan'] = True
@@ -1113,7 +1118,6 @@ def parse_args(args):
11131118

11141119
parser.add_argument(
11151120
'--stripped',
1116-
default=True,
11171121
action='store_true',
11181122
help='Strip debug symbols from the output. This defaults to true and has no effect on iOS.'
11191123
)

packages/flutter_tools/lib/src/android/gradle.dart

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ import 'migrations/top_level_gradle_build_file_migration.dart';
5252
final RegExp _kBuildVariantRegex = RegExp('^BuildVariant: (?<$_kBuildVariantRegexGroupName>.*)\$');
5353
const String _kBuildVariantRegexGroupName = 'variant';
5454
const String _kBuildVariantTaskName = 'printBuildVariants';
55+
@visibleForTesting
56+
const String failedToStripDebugSymbolsErrorMessage = r'''
57+
Release app bundle failed to strip debug symbols from native libraries. Please run flutter doctor and ensure that the Android toolchain does not report any issues.
58+
59+
Otherwise, file an issue at https://github.com/flutter/flutter/issues.''';
5560

5661
typedef _OutputParser = void Function(String line);
5762

@@ -86,6 +91,9 @@ Directory getBundleDirectory(FlutterProject project) {
8691
.childDirectory('bundle');
8792
}
8893

94+
@visibleForTesting
95+
final String apkAnalyzerBinaryName = globals.platform.isWindows ? 'apkanalyzer.bat' : 'apkanalyzer';
96+
8997
/// The directory where the repo is generated.
9098
/// Only applicable to AARs.
9199
Directory getRepoDirectory(Directory buildDirectory) {
@@ -577,6 +585,16 @@ class AndroidGradleBuilder implements AndroidBuilder {
577585

578586
if (isBuildingBundle) {
579587
final File bundleFile = findBundleFile(project, buildInfo, _logger, _analytics);
588+
589+
if ((buildInfo.mode == BuildMode.release) &&
590+
!(await _isAabStrippedOfDebugSymbols(
591+
project,
592+
bundleFile.path,
593+
androidBuildInfo.targetArchs,
594+
))) {
595+
throwToolExit(failedToStripDebugSymbolsErrorMessage);
596+
}
597+
580598
final String appSize =
581599
(buildInfo.mode == BuildMode.debug)
582600
? '' // Don't display the size when building a debug variant.
@@ -632,6 +650,64 @@ class AndroidGradleBuilder implements AndroidBuilder {
632650
}
633651
}
634652

653+
// Checks whether AGP has successfully stripped debug symbols from native libraries
654+
// - libflutter.so, aka the engine
655+
// - lib_app.so, aka the framework dart code
656+
// and moved them to the BUNDLE-METADATA directory. Block the build if this
657+
// isn't successful, as it means that debug symbols are getting included in
658+
// the final app that would be delivered to users.
659+
Future<bool> _isAabStrippedOfDebugSymbols(
660+
FlutterProject project,
661+
String aabPath,
662+
Iterable<AndroidArch> targetArchs,
663+
) async {
664+
if (globals.androidSdk == null) {
665+
_logger.printTrace(
666+
'Failed to find android sdk when checking final appbundle for debug symbols.',
667+
);
668+
return false;
669+
}
670+
if (!globals.androidSdk!.cmdlineToolsAvailable) {
671+
_logger.printTrace(
672+
'Failed to find cmdline-tools when checking final appbundle for debug symbols.',
673+
);
674+
return false;
675+
}
676+
final String? apkAnalyzerPath = globals.androidSdk!.getCmdlineToolsPath(apkAnalyzerBinaryName);
677+
if (apkAnalyzerPath == null) {
678+
_logger.printTrace(
679+
'Failed to find apkanalyzer when checking final appbundle for debug symbols.',
680+
);
681+
return false;
682+
}
683+
684+
final RunResult result = await _processUtils.run(
685+
<String>[apkAnalyzerPath, 'files', 'list', aabPath],
686+
workingDirectory: project.android.hostAppGradleRoot.path,
687+
environment: _java?.environment,
688+
);
689+
690+
if (result.exitCode != 0) {
691+
_logger.printTrace(
692+
'apkanalyzer finished with exit code 0 when checking final appbundle for debug symbols.\n'
693+
'stderr was: ${result.stderr}\n'
694+
'and stdout was: ${result.stdout}',
695+
);
696+
return false;
697+
}
698+
699+
// As long as libflutter.so.sym is present for at least one architecture,
700+
// assume AGP succeeded in stripping.
701+
if (result.stdout.contains('libflutter.so.sym')) {
702+
return true;
703+
}
704+
705+
_logger.printTrace(
706+
'libflutter.so.sym not present when checking final appbundle for debug symbols.',
707+
);
708+
return false;
709+
}
710+
635711
Future<void> _performCodeSizeAnalysis(
636712
String kind,
637713
File zipFile,

0 commit comments

Comments
 (0)