From df114fbe9b30afc94ee1154393aef8b575ee1612 Mon Sep 17 00:00:00 2001 From: Gray Mackall <34871572+gmackall@users.noreply.github.com> Date: Mon, 3 Feb 2025 09:07:25 -0800 Subject: [PATCH] Remove default for stripped option in `engine/src/flutter/tools/gn`, don't strip by default on android (#161546) Remake of https://github.com/flutter/engine/pull/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 https://github.com/flutter/flutter/pull/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 https://github.com/flutter/engine/pull/50443#issuecomment-1945644730, 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]. [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 --- engine/src/flutter/tools/gn | 8 +- .../flutter_tools/lib/src/android/gradle.dart | 76 ++++ .../android/android_gradle_builder_test.dart | 424 ++++++++++++++++++ 3 files changed, 506 insertions(+), 2 deletions(-) diff --git a/engine/src/flutter/tools/gn b/engine/src/flutter/tools/gn index 325b5cbece..34d780f3f3 100755 --- a/engine/src/flutter/tools/gn +++ b/engine/src/flutter/tools/gn @@ -666,7 +666,12 @@ def to_gn_args(args): if args.build_embedder_examples is not None: gn_args['build_embedder_examples'] = args.build_embedder_examples - gn_args['stripped_symbols'] = args.stripped + if args.stripped is True: + gn_args['stripped_symbols'] = True + elif args.stripped is False: + gn_args['stripped_symbols'] = False + else: + gn_args['stripped_symbols'] = args.target_os != 'android' if args.msan: gn_args['is_msan'] = True @@ -1113,7 +1118,6 @@ def parse_args(args): parser.add_argument( '--stripped', - default=True, action='store_true', help='Strip debug symbols from the output. This defaults to true and has no effect on iOS.' ) diff --git a/packages/flutter_tools/lib/src/android/gradle.dart b/packages/flutter_tools/lib/src/android/gradle.dart index a7e2124a70..832c80f6a2 100644 --- a/packages/flutter_tools/lib/src/android/gradle.dart +++ b/packages/flutter_tools/lib/src/android/gradle.dart @@ -52,6 +52,11 @@ import 'migrations/top_level_gradle_build_file_migration.dart'; final RegExp _kBuildVariantRegex = RegExp('^BuildVariant: (?<$_kBuildVariantRegexGroupName>.*)\$'); const String _kBuildVariantRegexGroupName = 'variant'; const String _kBuildVariantTaskName = 'printBuildVariants'; +@visibleForTesting +const String failedToStripDebugSymbolsErrorMessage = r''' +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. + +Otherwise, file an issue at https://github.com/flutter/flutter/issues.'''; typedef _OutputParser = void Function(String line); @@ -86,6 +91,9 @@ Directory getBundleDirectory(FlutterProject project) { .childDirectory('bundle'); } +@visibleForTesting +final String apkAnalyzerBinaryName = globals.platform.isWindows ? 'apkanalyzer.bat' : 'apkanalyzer'; + /// The directory where the repo is generated. /// Only applicable to AARs. Directory getRepoDirectory(Directory buildDirectory) { @@ -577,6 +585,16 @@ class AndroidGradleBuilder implements AndroidBuilder { if (isBuildingBundle) { final File bundleFile = findBundleFile(project, buildInfo, _logger, _analytics); + + if ((buildInfo.mode == BuildMode.release) && + !(await _isAabStrippedOfDebugSymbols( + project, + bundleFile.path, + androidBuildInfo.targetArchs, + ))) { + throwToolExit(failedToStripDebugSymbolsErrorMessage); + } + final String appSize = (buildInfo.mode == BuildMode.debug) ? '' // Don't display the size when building a debug variant. @@ -632,6 +650,64 @@ class AndroidGradleBuilder implements AndroidBuilder { } } + // Checks whether AGP has successfully stripped debug symbols from native libraries + // - libflutter.so, aka the engine + // - lib_app.so, aka the framework dart code + // and moved them to the BUNDLE-METADATA directory. Block the build if this + // isn't successful, as it means that debug symbols are getting included in + // the final app that would be delivered to users. + Future _isAabStrippedOfDebugSymbols( + FlutterProject project, + String aabPath, + Iterable targetArchs, + ) async { + if (globals.androidSdk == null) { + _logger.printTrace( + 'Failed to find android sdk when checking final appbundle for debug symbols.', + ); + return false; + } + if (!globals.androidSdk!.cmdlineToolsAvailable) { + _logger.printTrace( + 'Failed to find cmdline-tools when checking final appbundle for debug symbols.', + ); + return false; + } + final String? apkAnalyzerPath = globals.androidSdk!.getCmdlineToolsPath(apkAnalyzerBinaryName); + if (apkAnalyzerPath == null) { + _logger.printTrace( + 'Failed to find apkanalyzer when checking final appbundle for debug symbols.', + ); + return false; + } + + final RunResult result = await _processUtils.run( + [apkAnalyzerPath, 'files', 'list', aabPath], + workingDirectory: project.android.hostAppGradleRoot.path, + environment: _java?.environment, + ); + + if (result.exitCode != 0) { + _logger.printTrace( + 'apkanalyzer finished with exit code 0 when checking final appbundle for debug symbols.\n' + 'stderr was: ${result.stderr}\n' + 'and stdout was: ${result.stdout}', + ); + return false; + } + + // As long as libflutter.so.sym is present for at least one architecture, + // assume AGP succeeded in stripping. + if (result.stdout.contains('libflutter.so.sym')) { + return true; + } + + _logger.printTrace( + 'libflutter.so.sym not present when checking final appbundle for debug symbols.', + ); + return false; + } + Future _performCodeSizeAnalysis( String kind, File zipFile, diff --git a/packages/flutter_tools/test/general.shard/android/android_gradle_builder_test.dart b/packages/flutter_tools/test/general.shard/android/android_gradle_builder_test.dart index 8a38a12577..ff01b8f081 100644 --- a/packages/flutter_tools/test/general.shard/android/android_gradle_builder_test.dart +++ b/packages/flutter_tools/test/general.shard/android/android_gradle_builder_test.dart @@ -704,6 +704,430 @@ void main() { overrides: {AndroidStudio: () => FakeAndroidStudio()}, ); + group('Appbundle debug symbol tests', () { + final List commonCommandPortion = [ + 'gradlew', + '-q', + '-Ptarget-platform=android-arm64,android-arm,android-x64', + '-Ptarget=lib/main.dart', + '-Pbase-application-name=android.app.Application', + '-Pdart-obfuscation=false', + '-Ptrack-widget-creation=false', + '-Ptree-shake-icons=false', + ]; + + // Output from `/tools/bin/apkanalyzer files list ` + // on an aab not containing debug symbols. + const String apkanalyzerOutputWithoutSymFiles = r''' +/ +/META-INF/ +/META-INF/MANIFEST.MF +/META-INF/ANDROIDD.RSA +/META-INF/ANDROIDD.SF +/base/ +/base/root/ +/base/root/kotlin/ +/base/root/kotlin/reflect/ +/base/root/kotlin/reflect/reflect.kotlin_builtins +/base/root/kotlin/ranges/ +/base/root/kotlin/ranges/ranges.kotlin_builtins +/base/root/kotlin/kotlin.kotlin_builtins +/base/root/kotlin/internal/ +/base/root/kotlin/internal/internal.kotlin_builtins +/base/root/kotlin/coroutines/ +/base/root/kotlin/coroutines/coroutines.kotlin_builtins +/base/root/kotlin/collections/ +/base/root/kotlin/collections/collections.kotlin_builtins +/base/root/kotlin/annotation/ +/base/root/kotlin/annotation/annotation.kotlin_builtins +/base/root/kotlin-tooling-metadata.json +/base/root/META-INF/ +/base/root/META-INF/version-control-info.textproto +/base/root/META-INF/services/ +/base/root/META-INF/services/i0.b +/base/root/META-INF/services/i0.a +/base/root/META-INF/kotlinx_coroutines_core.version +/base/root/META-INF/kotlinx_coroutines_android.version +/base/root/META-INF/com/ +/base/root/META-INF/com/android/ +/base/root/META-INF/com/android/build/ +/base/root/META-INF/com/android/build/gradle/ +/base/root/META-INF/com/android/build/gradle/app-metadata.properties +/base/root/META-INF/androidx.window_window.version +/base/root/META-INF/androidx.window_window-java.version +/base/root/META-INF/androidx.window.extensions.core_core.version +/base/root/META-INF/androidx.viewpager_viewpager.version +/base/root/META-INF/androidx.versionedparcelable_versionedparcelable.version +/base/root/META-INF/androidx.tracing_tracing.version +/base/root/META-INF/androidx.startup_startup-runtime.version +/base/root/META-INF/androidx.savedstate_savedstate.version +/base/root/META-INF/androidx.profileinstaller_profileinstaller.version +/base/root/META-INF/androidx.loader_loader.version +/base/root/META-INF/androidx.lifecycle_lifecycle-viewmodel.version +/base/root/META-INF/androidx.lifecycle_lifecycle-viewmodel-savedstate.version +/base/root/META-INF/androidx.lifecycle_lifecycle-runtime.version +/base/root/META-INF/androidx.lifecycle_lifecycle-process.version +/base/root/META-INF/androidx.lifecycle_lifecycle-livedata.version +/base/root/META-INF/androidx.lifecycle_lifecycle-livedata-core.version +/base/root/META-INF/androidx.lifecycle_lifecycle-livedata-core-ktx.version +/base/root/META-INF/androidx.interpolator_interpolator.version +/base/root/META-INF/androidx.fragment_fragment.version +/base/root/META-INF/androidx.customview_customview.version +/base/root/META-INF/androidx.core_core.version +/base/root/META-INF/androidx.core_core-ktx.version +/base/root/META-INF/androidx.arch.core_core-runtime.version +/base/root/META-INF/androidx.annotation_annotation-experimental.version +/base/root/META-INF/androidx.activity_activity.version +/base/root/DebugProbesKt.bin +/base/resources.pb +/base/res/ +/base/res/mipmap-xxxhdpi-v4/ +/base/res/mipmap-xxxhdpi-v4/ic_launcher.png +/base/res/mipmap-xxhdpi-v4/ +/base/res/mipmap-xxhdpi-v4/ic_launcher.png +/base/res/mipmap-xhdpi-v4/ +/base/res/mipmap-xhdpi-v4/ic_launcher.png +/base/res/mipmap-mdpi-v4/ +/base/res/mipmap-mdpi-v4/ic_launcher.png +/base/res/mipmap-hdpi-v4/ +/base/res/mipmap-hdpi-v4/ic_launcher.png +/base/res/drawable-v21/ +/base/res/drawable-v21/launch_background.xml +/base/native.pb +/base/manifest/ +/base/manifest/AndroidManifest.xml +/base/lib/ +/base/lib/x86_64/ +/base/lib/x86_64/libflutter.so +/base/lib/x86_64/libapp.so +/base/lib/armeabi-v7a/ +/base/lib/armeabi-v7a/libflutter.so +/base/lib/armeabi-v7a/libapp.so +/base/lib/arm64-v8a/ +/base/lib/arm64-v8a/libflutter.so +/base/lib/arm64-v8a/libapp.so +/base/dex/ +/base/dex/classes.dex +/base/assets/ +/base/assets/flutter_assets/ +/base/assets/flutter_assets/shaders/ +/base/assets/flutter_assets/shaders/ink_sparkle.frag +/base/assets/flutter_assets/packages/ +/base/assets/flutter_assets/packages/cupertino_icons/ +/base/assets/flutter_assets/packages/cupertino_icons/assets/ +/base/assets/flutter_assets/packages/cupertino_icons/assets/CupertinoIcons.ttf +/base/assets/flutter_assets/fonts/ +/base/assets/flutter_assets/fonts/MaterialIcons-Regular.otf +/base/assets/flutter_assets/NativeAssetsManifest.json +/base/assets/flutter_assets/NOTICES.Z +/base/assets/flutter_assets/FontManifest.json +/base/assets/flutter_assets/AssetManifest.json +/base/assets/flutter_assets/AssetManifest.bin +/base/assets.pb +/BundleConfig.pb +/BUNDLE-METADATA/ +/BUNDLE-METADATA/com.android.tools.build.profiles/ +/BUNDLE-METADATA/com.android.tools.build.profiles/baseline.profm +/BUNDLE-METADATA/com.android.tools.build.profiles/baseline.prof +/BUNDLE-METADATA/com.android.tools.build.obfuscation/ +/BUNDLE-METADATA/com.android.tools.build.obfuscation/proguard.map +/BUNDLE-METADATA/com.android.tools.build.libraries/ +/BUNDLE-METADATA/com.android.tools.build.libraries/dependencies.pb +/BUNDLE-METADATA/com.android.tools.build.gradle/ +/BUNDLE-METADATA/com.android.tools.build.gradle/app-metadata.properties +'''; + + // Output from `/tools/bin/apkanalyzer files list ` + // on an aab containing debug symbols. + const String apkanalyzerOutputWithSymFiles = + apkanalyzerOutputWithoutSymFiles + + r''' +/BUNDLE-METADATA/com.android.tools.build.debugsymbols/ +/BUNDLE-METADATA/com.android.tools.build.debugsymbols/arm64-v8a/ +/BUNDLE-METADATA/com.android.tools.build.debugsymbols/arm64-v8a/libflutter.so.sym +'''; + + void createSharedGradleFiles() { + fileSystem.directory('android').childFile('build.gradle').createSync(recursive: true); + + fileSystem.directory('android').childFile('gradle.properties').createSync(recursive: true); + + fileSystem.directory('android').childDirectory('app').childFile('build.gradle') + ..createSync(recursive: true) + ..writeAsStringSync('apply from: irrelevant/flutter.gradle'); + } + + File createAabFile(BuildMode buildMode) { + final File aabFile = fileSystem + .directory('/build') + .childDirectory('app') + .childDirectory('outputs') + .childDirectory('bundle') + .childDirectory('$buildMode') + .childFile('app-$buildMode.aab'); + + aabFile.createSync(recursive: true); + + return aabFile; + } + + testUsingContext( + 'build succeeds when debug symbols present for at least one architecture', + () async { + final AndroidGradleBuilder builder = AndroidGradleBuilder( + java: FakeJava(), + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: Artifacts.test(), + analytics: fakeAnalytics, + gradleUtils: FakeGradleUtils(), + platform: FakePlatform(environment: {'HOME': '/home'}), + androidStudio: FakeAndroidStudio(), + ); + processManager.addCommand( + FakeCommand(command: List.of(commonCommandPortion)..add('bundleRelease')), + ); + + createSharedGradleFiles(); + final File aabFile = createAabFile(BuildMode.release); + final AndroidSdk sdk = AndroidSdk.locateAndroidSdk()!; + + processManager.addCommand( + FakeCommand( + command: [ + sdk.getCmdlineToolsPath(apkAnalyzerBinaryName)!, + 'files', + 'list', + aabFile.path, + ], + stdout: apkanalyzerOutputWithSymFiles, + ), + ); + + final FlutterProject project = FlutterProject.fromDirectoryTest( + fileSystem.currentDirectory, + ); + project.android.appManifestFile + ..createSync(recursive: true) + ..writeAsStringSync(minimalV2EmbeddingManifest); + + await builder.buildGradleApp( + project: project, + androidBuildInfo: const AndroidBuildInfo( + BuildInfo( + BuildMode.release, + null, + treeShakeIcons: false, + packageConfigPath: '.dart_tool/package_config.json', + ), + targetArchs: [ + AndroidArch.arm64_v8a, + AndroidArch.armeabi_v7a, + AndroidArch.x86_64, + ], + ), + target: 'lib/main.dart', + isBuildingBundle: true, + configOnly: false, + localGradleErrors: [], + ); + }, + overrides: {AndroidStudio: () => FakeAndroidStudio()}, + ); + + testUsingContext( + 'building a debug aab does not invoke apkanalyzer', + () async { + final AndroidGradleBuilder builder = AndroidGradleBuilder( + java: FakeJava(), + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: Artifacts.test(), + analytics: fakeAnalytics, + gradleUtils: FakeGradleUtils(), + platform: FakePlatform(environment: {'HOME': '/home'}), + androidStudio: FakeAndroidStudio(), + ); + processManager.addCommand( + FakeCommand(command: List.of(commonCommandPortion)..add('bundleDebug')), + ); + + createSharedGradleFiles(); + createAabFile(BuildMode.debug); + + final FlutterProject project = FlutterProject.fromDirectoryTest( + fileSystem.currentDirectory, + ); + project.android.appManifestFile + ..createSync(recursive: true) + ..writeAsStringSync(minimalV2EmbeddingManifest); + + await builder.buildGradleApp( + project: project, + androidBuildInfo: const AndroidBuildInfo( + BuildInfo( + BuildMode.debug, + null, + treeShakeIcons: false, + packageConfigPath: '.dart_tool/package_config.json', + ), + targetArchs: [ + AndroidArch.arm64_v8a, + AndroidArch.armeabi_v7a, + AndroidArch.x86_64, + ], + ), + target: 'lib/main.dart', + isBuildingBundle: true, + configOnly: false, + localGradleErrors: [], + ); + }, + overrides: {AndroidStudio: () => FakeAndroidStudio()}, + ); + + testUsingContext( + 'throws tool exit for missing debug symbols when building release app bundle', + () async { + final AndroidGradleBuilder builder = AndroidGradleBuilder( + java: FakeJava(), + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: Artifacts.test(), + analytics: fakeAnalytics, + gradleUtils: FakeGradleUtils(), + platform: FakePlatform(environment: {'HOME': '/home'}), + androidStudio: FakeAndroidStudio(), + ); + processManager.addCommand( + FakeCommand(command: List.of(commonCommandPortion)..add('bundleRelease')), + ); + + createSharedGradleFiles(); + final File aabFile = createAabFile(BuildMode.release); + + final AndroidSdk sdk = AndroidSdk.locateAndroidSdk()!; + + processManager.addCommand( + FakeCommand( + command: [ + sdk.getCmdlineToolsPath(apkAnalyzerBinaryName)!, + 'files', + 'list', + aabFile.path, + ], + stdout: apkanalyzerOutputWithoutSymFiles, + ), + ); + + final FlutterProject project = FlutterProject.fromDirectoryTest( + fileSystem.currentDirectory, + ); + project.android.appManifestFile + ..createSync(recursive: true) + ..writeAsStringSync(minimalV2EmbeddingManifest); + + await expectLater( + () async => builder.buildGradleApp( + project: project, + androidBuildInfo: const AndroidBuildInfo( + BuildInfo( + BuildMode.release, + null, + treeShakeIcons: false, + packageConfigPath: '.dart_tool/package_config.json', + ), + targetArchs: [ + AndroidArch.arm64_v8a, + AndroidArch.armeabi_v7a, + AndroidArch.x86_64, + ], + ), + target: 'lib/main.dart', + isBuildingBundle: true, + configOnly: false, + localGradleErrors: [], + ), + throwsToolExit(message: failedToStripDebugSymbolsErrorMessage), + ); + }, + overrides: {AndroidStudio: () => FakeAndroidStudio()}, + ); + + testUsingContext( + 'build aab in release mode fails when apkanalyzer exit code is non zero', + () async { + final AndroidGradleBuilder builder = AndroidGradleBuilder( + java: FakeJava(), + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: Artifacts.test(), + analytics: fakeAnalytics, + gradleUtils: FakeGradleUtils(), + platform: FakePlatform(environment: {'HOME': '/home'}), + androidStudio: FakeAndroidStudio(), + ); + processManager.addCommand( + FakeCommand(command: List.of(commonCommandPortion)..add('bundleRelease')), + ); + + createSharedGradleFiles(); + final File aabFile = createAabFile(BuildMode.release); + + final AndroidSdk sdk = AndroidSdk.locateAndroidSdk()!; + + processManager.addCommand( + FakeCommand( + command: [ + sdk.getCmdlineToolsPath(apkAnalyzerBinaryName)!, + 'files', + 'list', + aabFile.path, + ], + exitCode: 1, + stdout: apkanalyzerOutputWithSymFiles, + ), + ); + + final FlutterProject project = FlutterProject.fromDirectoryTest( + fileSystem.currentDirectory, + ); + project.android.appManifestFile + ..createSync(recursive: true) + ..writeAsStringSync(minimalV2EmbeddingManifest); + + await expectLater( + () async => builder.buildGradleApp( + project: project, + androidBuildInfo: const AndroidBuildInfo( + BuildInfo( + BuildMode.release, + null, + treeShakeIcons: false, + packageConfigPath: '.dart_tool/package_config.json', + ), + targetArchs: [ + AndroidArch.arm64_v8a, + AndroidArch.armeabi_v7a, + AndroidArch.x86_64, + ], + ), + target: 'lib/main.dart', + isBuildingBundle: true, + configOnly: false, + localGradleErrors: [], + ), + throwsToolExit(message: failedToStripDebugSymbolsErrorMessage), + ); + }, + overrides: {AndroidStudio: () => FakeAndroidStudio()}, + ); + }); + testUsingContext( 'indicates that an APK has been built successfully', () async {