Add split-debug and obfuscation to build aar #56342

jmagman · 2020-05-05T02:25:12Z

Description

Add --split-debug-info and --obfuscate flags to build aar.

Related Issues

Tests

build_aar_test flag parsing. Confirmed the tests also passed on master when I commented out the --split-debug-info and --obfuscate parts, so nothing regressed.

Add build aar to android_obfuscate_test` devicelab test.

Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

jmagman · 2020-05-05T02:26:14Z

packages/flutter_tools/lib/src/commands/build_aar.dart

-        ));
+        androidBuildInfo.add(
+          AndroidBuildInfo(
+            getBuildInfo(forcedBuildMode: BuildMode.fromName(buildMode)),


#56068 (comment)

The build_aar command would need to be refactored to get the buildInfo from the flutter command instead of manually constructing it.

Is this what you meant, @jonahwilliams?

Yup! though there are some edges here. For example, If a user builds a debug aar with obfuscation enabled, the sources might be scrambled. This might cause confusion if there is some expectation that these flags only work on "release" builds

@xster What do you think the best user experience would be here?

Ignore the flags for the debug and profile output

Ignore the flags for the debug and profile output, and print a warning

Only allow the flags accompanied by --no-debug and --no-profile

FWIW we do support --obfuscate on --debug flutter build apk --debug today, but not through flutter run. The former isn't very common so I didn't think it merited any interception, but it might be more common to build artifacts together here

I vote for 2

$ flutter build aar --target-platform=android-arm --obfuscate --split-debug-info=foo/ Building flutter tool... Changing current working directory to: /Users/magder/Projects/aaa Dart obfuscation is not supported in Debug mode, skipping. Running Gradle task 'assembleAarDebug'... Running Gradle task 'assembleAarDebug'... Done 20.6s ✓ Built build/host/outputs/repo. Dart obfuscation is not supported in Profile mode, skipping. Running Gradle task 'assembleAarProfile'... Running Gradle task 'assembleAarProfile'... Done 21.4s ✓ Built build/host/outputs/repo. Running Gradle task 'assembleAarRelease'... Running Gradle task 'assembleAarRelease'... Done 3.2s ✓ Built build/host/outputs/repo.

LG. "skipping" is a bit ambiguous since you didn't skip building debug/profile. Maybe just say building debug/release in unobfuscated mode?

Changed to: "Dart obfuscation is not supported in Debug mode, building as unobfuscated."

jmagman · 2020-05-05T02:29:58Z

packages/flutter_tools/test/general.shard/commands/build_aar_test.dart

+    });
+
+    tearDown(() {
+      tryToDelete(tempDir);


This isn't ideal, but it's the pattern the rest of this file uses. I started yak shaving (hence #56335, #56331, #56330, #56329) but didn't get far enough to do #43863 or inject a fake process manager into the command (boy are commands are hard to test...)

Anyway it's no worse than it was...

jmagman · 2020-05-05T02:35:45Z

Forgot to add the integration test.

jonahwilliams · 2020-05-05T20:11:10Z

packages/flutter_tools/test/general.shard/commands/build_aar_test.dart

+        expect(androidBuildInfo.targetArchs, <AndroidArch>[AndroidArch.armeabi_v7a, AndroidArch.arm64_v8a, AndroidArch.x86_64]);
+      }
+      expect(buildModes.length, 3);
+      expect(buildModes, contains(BuildMode.debug));


Consider https://api.flutter.dev/flutter/package-matcher_matcher/containsAll.html

jonahwilliams · 2020-05-05T20:12:39Z

packages/flutter_tools/test/general.shard/commands/build_aar_test.dart

+      expect(androidBuildInfos.length, 3);
+
+      final List<BuildMode> buildModes = <BuildMode>[];
+      for (final AndroidBuildInfo androidBuildInfo in androidBuildInfos) {


Possibly overly pedantic, but It would be nice to separate the control flow from the expectations here. You could nest contains and predicate/isA<> matchers to accomplish this, though I don't know if its an improvement

I tried it, I think it's harder to read...

jonahwilliams

LGTM with nit

jmagman · 2020-05-05T22:42:15Z

dev/devicelab/bin/tasks/android_obfuscate_test.dart

+        return TaskResult.failure('Found project name in obfuscated APK dart library');
+      }
+      if (foundAarProjectName) {
+        return TaskResult.failure('Found project name in obfuscated AAR dart library');


Bleh this fails:

[android_obfuscate_test] [STDOUT] Executing: grep aaa jni/armeabi-v7a/libapp.so in /var/folders/mv/qlhc4vsj6tqct166rw3zwmth00mfq2/T/flutter_devicelab_gradle_plugin_test.bY7bhQ/aaa [android_obfuscate_test] [STDOUT] stdout: Binary file jni/armeabi-v7a/libapp.so matches ... Task result: { "success": false, "reason": "Found project name in obfuscated AAR dart library" }

Does the build aar call into assemble in the same way?

xster · 2020-05-05T23:21:24Z

LGTM. Thanks for fixing!

jmagman · 2020-05-06T01:43:31Z

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

-    if (androidBuildInfo.buildInfo.treeShakeIcons) {
-      command.add('-Pfont-subset=true');
-    }


This is in the wrong spot, I moved it out of the local engine check.

@dnfield Can you confirm you didn't intentionally want the tree shake check to only happen with a local engine?

Correct - it should be able to work with both. I probably only tested with a local engine.

jmagman · 2020-05-06T01:47:19Z

packages/flutter_tools/test/general.shard/android/gradle_test.dart

+            '-Poutput-dir=${buildDirectory.path}',
+            '-Pis-plugin=true',
+            '-PbuildNumber=1.0',
+            '-q',


Added the -q flag.

jmagman added platform-android Android applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels May 5, 2020

jmagman requested review from xster and jonahwilliams May 5, 2020 02:25

jmagman self-assigned this May 5, 2020

googlebot added the cla: yes label May 5, 2020

jmagman commented May 5, 2020

View reviewed changes

jmagman added the work in progress; do not review label May 5, 2020

jonahwilliams reviewed May 5, 2020

View reviewed changes

jonahwilliams approved these changes May 5, 2020

View reviewed changes

jmagman added 2 commits May 5, 2020 15:40

Add split-debug and obfuscation to build aar

a85daad

Integration test

1737160

jmagman force-pushed the obfuscate-aar branch from b6dfd0c to 1737160 Compare May 5, 2020 22:41

jmagman commented May 5, 2020

View reviewed changes

Pass flag through to gradle

c293a01

jmagman commented May 6, 2020

View reviewed changes

containsAll

18bbb2d

jmagman commented May 6, 2020

View reviewed changes

Change warning text

166901f

jmagman removed the work in progress; do not review label May 7, 2020

Remove unused

ade3ac4

jmagman added the waiting for tree to go green label May 8, 2020

fluttergithubbot merged commit 0a4f6cd into flutter:master May 8, 2020

jmagman deleted the obfuscate-aar branch May 8, 2020 01:56

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Add split-debug and obfuscation to build aar #56342

Add split-debug and obfuscation to build aar #56342

Uh oh!

Conversation

jmagman commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented May 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented May 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman commented May 5, 2020 •

edited

Loading