Extract embedded proguard specs from JARs and AARs during R8

[snazhmudinov]

What has changed

Extract embedded proguard specs from JARs and AARs. Add support for extracting proguard specs embedded in META-INF/proguard/ and META-INF/com.android.tools/ — matching what AGP/R8 does by default.

What's new

• proguard_extractor_lib.py — shared library with extraction logic for both JAR and AAR archives
• jar_embedded_proguard_extractor — new tool for extracting specs from JARs (used by R8)
• aar_embedded_proguard_extractor — extended to optionally extract R8 rules from classes.jar

Feature flags (both default False)

• r8_extract_embedded_proguard_specs — enables embedded spec extraction from the deploy JAR during R8
• aar_import_extract_r8_rules — enables R8-targeted rule extraction from classes.jar in aar_import()

Both behaviors are opt-in to avoid breaking existing builds.

[snazhmudinov]

if the changes look good, I can squash them into one commit before merge.

[snazhmudinov]

@ted-xie can you check the latest changes I pushed?

[ted-xie]

I think some more context about how the PR import process works would be helpful here. The latest commits you pushed strayed a little far from the vision I had (I probably should have explained my reasoning better).

Background information

This repository is an export of the internal Starlark Android rules that Google uses in our monorepo. We use a tool called Copybara to 1) do a bunch of text-replace transformations; 2) copy translated files to Github. The text transformations are a mixed bag and can range from changing package import paths to changing the default values of attributes to even removing certain code paths entirely. There is a lot of stuff in the Google rules that is not applicable to OSS builds, and vice versa: there are some features that are OSS-specific that we don't want to enable internally. R8 compilation is one of those features that is specific to OSS builds; we have another code optimization tool for internal builds.

To import a PR, we have to pull these changes in via a reverse Copybara transform, which is sometimes tricky, but mostly straightforward. There are certain things that make the import more complicated, since we have to change the Copybara configuration to deal with them. File name changes or deletions are one of the change types that can make this process complicated.

The next step is always tricky and labor-intensive: we have to verify that newly-introduced code does not change how builds work for every single Android target in the entire monorepo. Not just binaries, but tests and libraries, too. This includes huge apps like Gmail, Maps, etc. The more logic that a PR touches, especially in intricate parts of the codebase such as resource processing or proguard spec mangling, the more likely it is that the PR will cause an app breakage. We have had to permanently revert community PRs before because they broke internal apps. Just setting up the monorepo-wide tests can take a day+, not to mention running the tests (takes a whole day), and triaging breakages.

This PR

Your proposed change has a lot of features that complicate the import considerably.

• Renaming a toolchain attribute: This isn't the worst thing, but a little unnecessary (I'll elaborate below)
• Renaming a tool and adding new features to it
  • Since git doesn't really support file "move" changes, the diff appears as a deletion and new file addition, so it's impossible for me to see what actually changed. You might have only changed 4 lines, but it appears as ~300 new lines, which is unreviewable.
• Unconditionally changing how AAR proguard collection works. Maybe this works for your particular usecase, but it might cause proguard rule forwarding behavior to change for internal apps, not to mention non-Google apps that depend on this codebase.

My suggestions

Given the background information and my notes above, I think there are a few things we could do to still try and get this change imported.

• I would much rather see a change where much of the business logic in tools/android/aar_embedded_proguard_extractor.py is abstracted to a library, since that logic would be common to proguard spec extraction for both AARs and JARs. Any new additional behavior for jar proguard extraction could be added on top of the core business logic, and any new jar_proguard_extractor_tool would basically be a thin shell around the library call. This minimizes logic duplication and follows general conventions for behavior encapsulation.
  • You'll still probably have to add a new toolchain attribute, but attr additions are much easier to test and vet than renames for us.
• The new logic you've added in aar_import() should be hidden behind some kind of feature flag. Is it possible to check within aar_import() whether R8 was used for compilation? If not, please consider adding a Starlark flag that optionally enables this behavior.

[snazhmudinov]

I think some more context about how the PR import process works would be helpful here. The latest commits you pushed strayed a little far from the vision I had (I probably should have explained my reasoning better).

Background information

This repository is an export of the internal Starlark Android rules that Google uses in our monorepo. We use a tool called Copybara to 1) do a bunch of text-replace transformations; 2) copy translated files to Github. The text transformations are a mixed bag and can range from changing package import paths to changing the default values of attributes to even removing certain code paths entirely. There is a lot of stuff in the Google rules that is not applicable to OSS builds, and vice versa: there are some features that are OSS-specific that we don't want to enable internally. R8 compilation is one of those features that is specific to OSS builds; we have another code optimization tool for internal builds.

To import a PR, we have to pull these changes in via a reverse Copybara transform, which is sometimes tricky, but mostly straightforward. There are certain things that make the import more complicated, since we have to change the Copybara configuration to deal with them. File name changes or deletions are one of the change types that can make this process complicated.

The next step is always tricky and labor-intensive: we have to verify that newly-introduced code does not change how builds work for every single Android target in the entire monorepo. Not just binaries, but tests and libraries, too. This includes huge apps like Gmail, Maps, etc. The more logic that a PR touches, especially in intricate parts of the codebase such as resource processing or proguard spec mangling, the more likely it is that the PR will cause an app breakage. We have had to permanently revert community PRs before because they broke internal apps. Just setting up the monorepo-wide tests can take a day+, not to mention running the tests (takes a whole day), and triaging breakages.

This PR

Your proposed change has a lot of features that complicate the import considerably.

• Renaming a toolchain attribute: This isn't the worst thing, but a little unnecessary (I'll elaborate below)

• Renaming a tool and adding new features to it

  • Since git doesn't really support file "move" changes, the diff appears as a deletion and new file addition, so it's impossible for me to see what actually changed. You might have only changed 4 lines, but it appears as ~300 new lines, which is unreviewable.

• Unconditionally changing how AAR proguard collection works. Maybe this works for your particular usecase, but it might cause proguard rule forwarding behavior to change for internal apps, not to mention non-Google apps that depend on this codebase.

My suggestions

Given the background information and my notes above, I think there are a few things we could do to still try and get this change imported.

• I would much rather see a change where much of the business logic in tools/android/aar_embedded_proguard_extractor.py is abstracted to a library, since that logic would be common to proguard spec extraction for both AARs and JARs. Any new additional behavior for jar proguard extraction could be added on top of the core business logic, and any new jar_proguard_extractor_tool would basically be a thin shell around the library call. This minimizes logic duplication and follows general conventions for behavior encapsulation.

  • You'll still probably have to add a new toolchain attribute, but attr additions are much easier to test and vet than renames for us.

• The new logic you've added in aar_import() should be hidden behind some kind of feature flag. Is it possible to check within aar_import() whether R8 was used for compilation? If not, please consider adding a Starlark flag that optionally enables this behavior.

Thanks for the clarification! I have pushed the changes based on the proposed requirements, please let me if this approach is better.

[snazhmudinov]

If the changes are still not compatible with internal rules, maybe we can then just expose 2 entry points in the android toolchain - one for aar extractor and one for jar extractor, guard their usage with feature flags, and I can override those toolchain entries with my own tools.