New issue
Advanced search Search tips

Issue 882524 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Make it easier to see when Proguard rules are being added to an apk

Project Member Reported by agrieve@chromium.org, Sep 10

Issue description

Problem:
Proguard rules can be added to any java library target, but can contain rules that affect globally. They are also notoriously hard to write properly.

Solution:
Allow android_apk targets to specify an "expected" merged proguard rules file, and have the build fail if the rules don't match.

This will mean an expectations file will need to be updated when any library tweaks their proguard rules.

I'm thinking we should enable this for chrome_public_apk, but not for downstream targets (to avoid downstream breaking too often). Although, perhaps we could have downstream expectations "extend" upstream ones? Or allow multiple files to be listed as expectations?

Filed similar bug for AndroidManifest.xml: bug 882528
 
Description: Show this description
Cc: estevenson@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2506a433b96350f943d72f684f9a05feba91aba4

commit 2506a433b96350f943d72f684f9a05feba91aba4
Author: Eric Stevenson <estevenson@chromium.org>
Date: Thu Jan 10 18:50:02 2019

Android: Support for tracking proguard flags changes.

This CL adds build rules to support verifying that certain generated
files haven't changed. Currently the merged proguard flags file is
supported.

This is only enforced when check_android_configuration = true, which
is only enabled on the android-binary-size trybot.

Follow-up CLs will include:
  * Support for "extending" expectation files (for use downstream)
  * Support for APK AndroidManifest.xml files

Bug: 882524, 882528
Change-Id: I421e1f24788a11343df5ad91cde7cc86ec15c801
Reviewed-on: https://chromium-review.googlesource.com/c/1394998
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621673}
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/android/gyp/proguard.py
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/android/gyp/proguard.pydeps
[add] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/android/gyp/util/diff_utils.py
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/config/android/config.gni
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/config/android/internal_rules.gni
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/build/config/android/rules.gni
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/chrome/android/BUILD.gn
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/chrome/android/OWNERS
[add] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/chrome/android/java/chrome_modern_public_apk.proguard_flags.expected
[add] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/chrome/android/java/chrome_public_apk.proguard_flags.expected
[add] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/chrome/android/java/monochrome_public_apk.proguard_flags.expected
[modify] https://crrev.com/2506a433b96350f943d72f684f9a05feba91aba4/tools/mb/mb_config.pyl

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/753eedddefcfcb94380b72c5b50a76430b94982b

commit 753eedddefcfcb94380b72c5b50a76430b94982b
Author: Andrew Grieve <agrieve@chromium.org>
Date: Fri Jan 11 21:59:01 2019

Android: Don't check public apk proguard expectations when downstream play services is present

Noticed that the expectations were failing for no-op builds because of
this.

Bug: 882524
Change-Id: If0297c5e0ae0db61529089b6afbaaa430fa1ee01
Reviewed-on: https://chromium-review.googlesource.com/c/1407597
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622173}
[modify] https://crrev.com/753eedddefcfcb94380b72c5b50a76430b94982b/build/android/gyp/util/diff_utils.py
[modify] https://crrev.com/753eedddefcfcb94380b72c5b50a76430b94982b/chrome/android/BUILD.gn

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa454023ebfef80893cc5ab612cc332511c6bc26

commit fa454023ebfef80893cc5ab612cc332511c6bc26
Author: Eric Stevenson <estevenson@chromium.org>
Date: Wed Jan 16 22:08:59 2019

Android: Make proguard expectations files easier to use/update.

Changes:
  * Add //chrome/android/java/README.md with better instructions
  * Update the error message to point at the README
  * Only enable expected files for monochrome_public_apk (changes now
    only require building 1 slow target instead of 3)
  * Generate the merge config file from input configs instead of using
    "-printconfiguration" so that we can exclude configs generated for
    resources (these change often and don't add any value)

Bug: 882524
Change-Id: Ib5e59636b8e5b7ae24be4004d4e27a3069575b15
Reviewed-on: https://chromium-review.googlesource.com/c/1412502
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623392}
[modify] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/build/android/gyp/proguard.py
[modify] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/build/android/gyp/util/diff_utils.py
[modify] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/build/android/gyp/util/proguard_util.py
[modify] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/chrome/android/BUILD.gn
[add] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/chrome/android/java/README.md
[delete] https://crrev.com/cdcad69f5500551ddf133b715a333ea82863d176/chrome/android/java/chrome_modern_public_apk.proguard_flags.expected
[delete] https://crrev.com/cdcad69f5500551ddf133b715a333ea82863d176/chrome/android/java/chrome_public_apk.proguard_flags.expected
[modify] https://crrev.com/fa454023ebfef80893cc5ab612cc332511c6bc26/chrome/android/java/monochrome_public_apk.proguard_flags.expected

Comment 6 by agrieve@chromium.org, Jan 21 (2 days ago)

Cc: -estevenson@chromium.org
Owner: estevenson@chromium.org
Status: Started (was: Available)

Comment 7 by agrieve@chromium.org, Yesterday (41 hours ago)

Labels: binary_size_team_q1_2019

Sign in to add a comment