Enable Proguard by default on release mode by blasten · Pull Request #39986 · flutter/flutter

blasten · 2019-09-06T17:01:32Z

Description

Add --proguard flag to flutter build apk and flutter build appbundle and enable it by default for release builts.

This change makes it possible to add custom rules by placing the file proguard-rules.pro in the android/ directory in case a plugin requires special rules.

Size in bytes

	hello_world	flutter_gallery
with proguard	4,473,289	41,522,555
without proguard	4,524,765	41,820,143

cc @Hixie

Related Issues

Fixes #11099

Tests

build_apk_test.dart
build_appbundle_test.dart

These tests aren't hermetic yet, but these are the first ones to test the calls to Gradle.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

codecov · 2019-09-06T17:47:54Z

Codecov Report

Merging #39986 into master will decrease coverage by 0.23%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##           master   #39986      +/-   ##
==========================================
- Coverage   58.56%   58.33%   -0.24%     
==========================================
  Files         192      194       +2     
  Lines       18443    18749     +306     
==========================================
+ Hits        10801    10937     +136     
- Misses       7642     7812     +170

Flag	Coverage Δ
#flutter_tool	`58.33% <78.26%> (-0.24%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/build_info.dart	`77.04% <ø> (+1.63%)`	⬆️
...lutter_tools/lib/src/commands/build_appbundle.dart	`100% <100%> (ø)`	⬆️
packages/flutter_tools/lib/src/context_runner.dart	`71.69% <100%> (+0.54%)`	⬆️
...ages/flutter_tools/lib/src/commands/build_apk.dart	`100% <100%> (ø)`	⬆️
packages/flutter_tools/lib/src/android/gradle.dart	`74.27% <74.35%> (+11.88%)`	⬆️
...ackages/flutter_tools/lib/src/commands/doctor.dart	`28.57% <0%> (-35.72%)`	⬇️
...lutter_tools/lib/src/android/android_emulator.dart	`32.65% <0%> (-30.62%)`	⬇️
.../flutter_tools/lib/src/windows/windows_device.dart	`30% <0%> (-20%)`	⬇️
...lib/src/build_runner/web_compilation_delegate.dart	`14.15% <0%> (-11.24%)`	⬇️
...ackages/flutter_tools/lib/src/reporting/usage.dart	`77.85% <0%> (-10.74%)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1222926...f2ecef3. Read the comment docs.

blasten · 2019-09-06T18:41:39Z

Now I just realized that coverage report has false positives. The decreased coverage in some files is just the result of moving code, so it can be reused.

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

dnfield

couple nits - was there some reason we didn't enable this in the past? I was under the impression it could be destructive in some way for plugins that would be non-obvious for new users. I see a way to override that for plugins here, but will that be something that (new) users will be able to understand and use correctly?

packages/flutter_tools/gradle/flutter_proguard_rules.pro

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

packages/flutter_tools/test/src/common.dart

blasten · 2019-09-09T18:13:45Z

If Proguard failed, the tool will log the event and tell the user to use --no-proguard.

If we detect that a lot of folks are seeing warnings from Proguard, we could flip the default value and scoped down the issues. For instance: #37839.

dnfield · 2019-09-09T18:34:18Z

Wouldn't the failure potentially happen at runtime though? E.g. if proguard removed a class you actually want to use but it thought was safe to remove.

blasten · 2019-09-09T20:41:36Z

@dnfield I can only think of JNI calls or reflection. This won't be an issue in the engine since the rules are configured, but if a plugin is doing something similar then that plugin should use the @Keep annotation or provide Proguard rules. Are you aware of any plugin that might be doing something like that? This might need better docs from our side.

dnfield · 2019-09-09T21:13:51Z

I'm not sure about any plugins using reflection. I may be mis-remembering, but I had a vague notion that the way we do the plugin registration could end up having proguard shake out plugin code we want to keep.

Is there a test that covers this flag with a plugin enabled?

blasten · 2019-09-09T21:21:49Z

Is there a test that covers this flag with a plugin enabled?

yes, the Flutter gallery. I also tested manually and on Firebase device lab.

we do the plugin registration could end up having proguard shake out plugin code we want to keep.

AFAICT, GeneratedPluginRegistrant imports each of the plugins and calls the registerWith method on the main plugin class.

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

dnfield

LGTM with nit

zanderso

lgtm w/ nits

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

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

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

rostopira · 2019-09-11T10:03:01Z

This broke my build:

Initializing gradle...                                              0.5s
Resolving dependencies...                                           2.5s
                                                                        
FAILURE: Build failed with an exception.                                
                                                                        
* Where:                                                                
Script '/Users/rostopira/flutter/packages/flutter_tools/gradle/flutter.gradle' line: 163
                                                                        
* What went wrong:                                                      
A problem occurred evaluating script.                                   
> Failed to apply plugin [class 'FlutterPlugin']                        
   > No signature of method: static FlutterPlugin.useProguard() is applicable for argument types: (Boolean) values: [true]
                                                                        
* Try:                                                                  
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
                                                                        
* Get more help at https://help.gradle.org

blasten · 2019-09-11T14:38:05Z

@rostopira what version of Gradle and the Android Gradle plugin are you using?

This issue is being discussed on #40213

This reverts commit f098de1.

Enable Proguard by default on release mode

c1953d3

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 6, 2019

googlebot added the cla: yes label Sep 6, 2019

Add singlethon to GradleUtils

2d220d1

Emmanuel Garcia added 7 commits September 6, 2019 17:04

Fix tests

312d3aa

Tweaks

c464422

Expect specific ToolExit

5f102cc

Mock AndroidSdk

591f1f7

Fix test on Windows

6473834

Use gradlew.bat on Windows

f68fc0b

Use gradlew.bat on Windows - App Bundle

7cb0dc4

blasten requested review from dnfield, jonahwilliams and zanderso September 9, 2019 15:48

jonahwilliams reviewed Sep 9, 2019

View reviewed changes

packages/flutter_tools/lib/src/android/gradle.dart Outdated Show resolved Hide resolved

dnfield reviewed Sep 9, 2019

View reviewed changes

packages/flutter_tools/gradle/flutter_proguard_rules.pro Outdated Show resolved Hide resolved

packages/flutter_tools/lib/src/android/gradle.dart Outdated Show resolved Hide resolved

packages/flutter_tools/test/src/common.dart Show resolved Hide resolved

Address comments

48da4d9

blasten requested review from dnfield and jonahwilliams September 9, 2019 18:14

dnfield reviewed Sep 9, 2019

View reviewed changes

packages/flutter_tools/lib/src/android/gradle.dart Show resolved Hide resolved

dnfield approved these changes Sep 9, 2019

View reviewed changes

Improve comment

c3985b7

blasten force-pushed the proguard branch from 7c285d8 to c3985b7 Compare September 10, 2019 01:20

zanderso approved these changes Sep 10, 2019

View reviewed changes

Remove emojis

f2ecef3

blasten force-pushed the proguard branch from 69eafd7 to f2ecef3 Compare September 10, 2019 20:33

blasten merged commit f098de1 into flutter:master Sep 11, 2019

blasten deleted the proguard branch September 11, 2019 00:23

blasten mentioned this pull request Sep 11, 2019

Reland #39157 #39798

Merged

amirh added a commit that referenced this pull request Sep 11, 2019

Revert "Enable Proguard by default on release mode (#39986)"

4c3dbed

This reverts commit f098de1.

amirh mentioned this pull request Sep 11, 2019

Revert "Enable Proguard by default on release mode" #40273

Closed

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Enable Proguard by default on release mode (flutter#39986)

624f6fe

jmagman mentioned this pull request Jan 18, 2020

Proguard configuration when use google_sign_in (3.0.4) plugin #19102

Closed

jmagman mentioned this pull request Mar 23, 2021

Some plugins breaks on release mode on Android #78625

Closed

arjunbansal mentioned this pull request Apr 21, 2021

Question about Facebook app install ads on Android RodrigoSMarques/flutter_branch_sdk#90

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Conversation

blasten commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Size in bytes

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

codecov bot commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blasten commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blasten commented Sep 9, 2019

Uh oh!

dnfield commented Sep 9, 2019

Uh oh!

blasten commented Sep 9, 2019

Uh oh!

dnfield commented Sep 9, 2019

Uh oh!

blasten commented Sep 9, 2019

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rostopira commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blasten commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

blasten commented Sep 6, 2019 •

edited

Loading

codecov bot commented Sep 6, 2019 •

edited

Loading

blasten commented Sep 6, 2019 •

edited

Loading

rostopira commented Sep 11, 2019 •

edited

Loading

blasten commented Sep 11, 2019 •

edited

Loading