Reland #39157 #39798

blasten · 2019-09-04T06:20:59Z

bdd27ea is the re-land of Reland #39157 #39798
de2e98b is the fix for Maven dependency error on flutter upgrade #39727

I reverted the commits that enabled proguard since #39986 already enabled Proguard.

The APK size increase (examples/hello_world): 5.81KB

)" This reverts commit d230234.

blasten · 2019-09-04T06:43:29Z

The tool generates a .java file that passes the fully qualified name of the plugin's main class to the registry: https://github.com/flutter/engine/blob/0a0f3305b5f086b3a413c5156bce810e1d6f4a32/shell/platform/android/io/flutter/plugin/common/PluginRegistry.java#L34

codecov · 2019-09-04T07:00:37Z

Codecov Report

Merging #39798 into master will increase coverage by 1.29%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master   #39798      +/-   ##
==========================================
+ Coverage   56.64%   57.93%   +1.29%     
==========================================
  Files         194      194              
  Lines       18691    18729      +38     
==========================================
+ Hits        10587    10851     +264     
+ Misses       8104     7878     -226

Flag	Coverage Δ
#flutter_tool	`57.93% <82.35%> (+1.29%)`	⬆️

Impacted Files	Coverage Δ
...ackages/flutter_tools/lib/src/commands/doctor.dart	`64.28% <ø> (ø)`	⬆️
.../flutter_tools/lib/src/runner/flutter_command.dart	`80.7% <ø> (ø)`	⬆️
...ages/flutter_tools/lib/src/commands/build_apk.dart	`100% <ø> (ø)`	⬆️
...ages/flutter_tools/lib/src/commands/build_aar.dart	`93.33% <ø> (-0.22%)`	⬇️
...kages/flutter_tools/lib/src/commands/precache.dart	`93.1% <100%> (+1.43%)`	⬆️
packages/flutter_tools/lib/executable.dart	`92.85% <100%> (ø)`	⬆️
...ckages/flutter_tools/lib/src/commands/upgrade.dart	`43.82% <100%> (+9.33%)`	⬆️
packages/flutter_tools/lib/src/android/gradle.dart	`62.39% <62.5%> (ø)`	⬆️
packages/flutter_tools/lib/src/cache.dart	`45.43% <82.35%> (+1.2%)`	⬆️
packages/flutter_tools/lib/src/version.dart	`90.9% <0%> (-1.92%)`	⬇️
... and 11 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 72cacb4...de2e98b. Read the comment docs.

codecov · 2019-09-04T07:00:37Z

Codecov Report

Merging #39798 into master will increase coverage by 0.33%.
The diff coverage is 84.52%.

@@            Coverage Diff             @@
##           master   #39798      +/-   ##
==========================================
+ Coverage   59.56%   59.89%   +0.33%     
==========================================
  Files         192      192              
  Lines       18705    18742      +37     
==========================================
+ Hits        11141    11226      +85     
+ Misses       7564     7516      -48

Flag	Coverage Δ
#flutter_tool	`59.89% <84.52%> (+0.33%)`	⬆️

Impacted Files	Coverage Δ
...ackages/flutter_tools/lib/src/commands/doctor.dart	`64.28% <ø> (ø)`	⬆️
.../flutter_tools/lib/src/runner/flutter_command.dart	`81.54% <ø> (+3%)`	⬆️
...ages/flutter_tools/lib/src/commands/build_apk.dart	`100% <ø> (ø)`	⬆️
...ages/flutter_tools/lib/src/commands/build_aar.dart	`93.33% <ø> (-0.22%)`	⬇️
...ckages/flutter_tools/lib/src/commands/upgrade.dart	`43.47% <100%> (+9.03%)`	⬆️
...kages/flutter_tools/lib/src/commands/precache.dart	`93.1% <100%> (+1.43%)`	⬆️
packages/flutter_tools/lib/executable.dart	`92.85% <100%> (ø)`	⬆️
packages/flutter_tools/lib/src/cache.dart	`45.31% <82.08%> (+1.08%)`	⬆️
packages/flutter_tools/lib/src/android/gradle.dart	`75.14% <87.5%> (ø)`	⬆️
...utter_tools/lib/src/build_runner/build_script.dart	`16.19% <0%> (-3.81%)`	⬇️
... and 8 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 0df1594...a6589db. Read the comment docs.

Zazo032 · 2019-09-04T09:01:15Z

packages/flutter_tools/templates/app/android-java.tmpl/app/build.gradle.tmpl

@@ -51,6 +51,9 @@ android {
            // TODO: Add your own signing config for the release build.
            // Signing with the debug keys for now, so `flutter run --release` works.
            signingConfig signingConfigs.debug
+            minifyEnabled true
+            useProguard true
+            proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'


What would be the difference between adding proguard-android.txt instead of proguard-android-optimize.txt?

It looks like proguard-android-optimize.txt turns on optimizations that could cause issues on some versions of Dalvik.

https://android.googlesource.com/platform/sdk/+/master/files/proguard-android-optimize.txt#4

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

packages/flutter_tools/test/general.shard/cache_test.dart

zanderso · 2019-09-04T15:27:33Z

The docs indicate that build times may be affected. I wonder if @chinmaygarde or @cbracken have any of the context here.

Hixie · 2019-09-04T21:51:24Z

So this makes things worse by 153,180 bytes. That seems like a lot.

Hixie · 2019-09-04T21:52:21Z

Release mode build times being affected is a non-issue so long as they don't become an order of magnitude slower.

Hixie · 2019-09-04T21:54:34Z

examples/catalog/android/app/build.gradle

@@ -45,6 +45,9 @@ android {
            // TODO: Add your own signing config for the release build.
            // Signing with the debug keys for now, so `flutter run --release` works.
            signingConfig signingConfigs.debug
+            minifyEnabled true
+            useProguard true
+            proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'
        }
    }
 }


Is there some way we can do this that doesn't involve developers having to edit their build.grade files?

We could configure the setting from the Flutter plugin, so developers don't have to set this up. However, we do need to provide a way to edit the proguard rules in the file proguard-rules.pro, in case a plugin needs special rules.

blasten · 2019-09-04T23:56:22Z

I broke down the dependencies by size:

Dependency	Size (in bytes)
com.android.support:support-fragment	66,550
com.android.support:support-v13	47,818
android.arch.lifecycle:runtime	4,793
android.arch.lifecycle:common-java8	128

@Hixie if you think this is a non-starter, then the next option is separating the embedding into two dependencies such that the full app APK doesn't include support-fragment and support-v13. Doing so would bring the size down to 4.3MB (4,558,819).

… examples" This reverts commit e38a9a5.

This reverts commit e7a6833.

blasten · 2019-09-11T06:12:49Z

The APK size will increase by just 5.81KB, which makes sense since that is the size of lifecycles according to this table: #39798 (comment) cc @xster

blasten · 2019-09-11T06:13:15Z

PTAL

xster · 2019-09-11T06:34:15Z

It was a bit hard to figure out the context since there was a master merge in the middle of the commits but I'm assuming I'm just reviewing bdd27ea...blasten:e38a9a5b3d034862d1448b95f792a619f283c8bc and 7e95146...blasten:reland_maven_deps (done since you merged proguard enable separately) on top of the revert of your previous revert?

If so LGTM. wrt size, we discussed offline and 5.8kb seems reasonable since the current APIs straight up don't implement large swaths of plugin functionalities that iOS implements https://github.com/flutter/engine/blob/c58c5939e95f8d0232e60d0db9f00bc510caec99/shell/platform/darwin/ios/framework/Headers/FlutterPlugin.h#L49. So whether we wrote bridging code ourselves or pulled in a dependency, 5.8kb seems like the right ballpark.

zanderso · 2019-09-11T16:39:33Z

What was the reason for the revert? Can you note what, if any, fixes were added for the re-land?

blasten · 2019-09-11T16:50:56Z

@zanderso 👍I just updated the description.

Revert "Revert Maven dependencies (flutter#39747) (39157) (flutter#39784

bdd27ea

)" This reverts commit d230234.

fluttergithubbot added d: examples Sample code and demos team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 4, 2019

googlebot added the cla: yes label Sep 4, 2019

blasten requested review from jonahwilliams and zanderso September 4, 2019 06:21

Emmanuel Garcia added 2 commits September 3, 2019 23:30

Fix flutter#39758: Enable proguard on release mode

e7a6833

Fix flutter#39727

de2e98b

blasten force-pushed the reland_maven_deps branch from 1d8588e to de2e98b Compare September 4, 2019 06:31

blasten mentioned this pull request Sep 4, 2019

flutter build appbundle failing - Execution failed for task ':app:transformClassesAndResourcesWithProguardForRelease'. #39650

Closed

Zazo032 reviewed Sep 4, 2019

View reviewed changes

zanderso reviewed Sep 4, 2019

View reviewed changes

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

packages/flutter_tools/test/general.shard/cache_test.dart Show resolved Hide resolved

Emmanuel Garcia added 2 commits September 4, 2019 09:08

Remove line and sort imports

97daec6

Add proguard rules to keep classes called from C++ and update examples

e38a9a5

This comment has been minimized.

Sign in to view

Hixie reviewed Sep 4, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Emmanuel Garcia added 5 commits September 10, 2019 18:26

Merge branch 'master' into reland_maven_deps

7e95146

Make useProguard() a class method

0d0d5e7

Revert "Add proguard rules to keep classes called from C++ and update…

0b40345

… examples" This reverts commit e38a9a5.

Revert "Fix flutter#39758: Enable proguard on release mode"

ef76b6b

This reverts commit e7a6833.

Fix tests

7cc418b

blasten force-pushed the reland_maven_deps branch from 082fcc3 to 7cc418b Compare September 11, 2019 04:08

blasten requested review from zanderso and Hixie September 11, 2019 06:12

Merge branch 'master' into reland_maven_deps

a6f1639

zanderso approved these changes Sep 11, 2019

View reviewed changes

blasten added the waiting for tree to go green label Sep 16, 2019

Merge branch 'master' into reland_maven_deps

e4f8874

blasten force-pushed the reland_maven_deps branch from d3e4d1c to e4f8874 Compare September 16, 2019 17:18

Emmanuel Garcia added 3 commits September 16, 2019 11:46

Fix AndroidMavenArtifacts test

4a92199

Merge branch 'master' into reland_maven_deps

d1cf022

Don't resolve transitive dependencies in plugins

a6589db

blasten merged commit 8a1bf5b into flutter:master Sep 17, 2019

blasten deleted the reland_maven_deps branch September 17, 2019 15:19

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

Reland flutter#39157 (flutter#39798)

273a43b

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland #39157 #39798

Reland #39157 #39798

blasten commented Sep 4, 2019 •

edited

blasten commented Sep 4, 2019

codecov bot commented Sep 4, 2019

codecov bot commented Sep 4, 2019 •

edited

Zazo032 Sep 4, 2019

blasten Sep 4, 2019

zanderso commented Sep 4, 2019

This comment has been minimized.

Hixie commented Sep 4, 2019

Hixie commented Sep 4, 2019

Hixie Sep 4, 2019

blasten Sep 4, 2019

blasten commented Sep 4, 2019

This comment has been minimized.

blasten commented Sep 11, 2019

blasten commented Sep 11, 2019

xster commented Sep 11, 2019

zanderso commented Sep 11, 2019

blasten commented Sep 11, 2019

Reland #39157 #39798

Reland #39157 #39798

Conversation

blasten commented Sep 4, 2019 • edited

blasten commented Sep 4, 2019

codecov bot commented Sep 4, 2019

Codecov Report

codecov bot commented Sep 4, 2019 • edited

Codecov Report

Zazo032 Sep 4, 2019

Choose a reason for hiding this comment

blasten Sep 4, 2019

Choose a reason for hiding this comment

zanderso commented Sep 4, 2019

This comment has been minimized.

Hixie commented Sep 4, 2019

Hixie commented Sep 4, 2019

Hixie Sep 4, 2019

Choose a reason for hiding this comment

blasten Sep 4, 2019

Choose a reason for hiding this comment

blasten commented Sep 4, 2019

This comment has been minimized.

blasten commented Sep 11, 2019

blasten commented Sep 11, 2019

xster commented Sep 11, 2019

zanderso commented Sep 11, 2019

blasten commented Sep 11, 2019

blasten commented Sep 4, 2019 •

edited

codecov bot commented Sep 4, 2019 •

edited