Configure ProGuard for Android

[cpsauer]

Please see #14909 for context :)

Of course, feel free to modify as you see fit if that'd be faster overall.
It's my first time touching a lot of these parts of the codebase--so I'm hoping you'll be patient with me.

Fixes #14909

Thanks for reading and for all you do!
Chris
(ex-Googler)

[Bencodes]

Cc @ahumesky

[cpsauer]

cc'ing also @comius, since he was assigned my other, fairly related pull requests

[comius]

cc'ing also @comius, since he was assigned my other, fairly related pull requests
Thanks.

This PR seems correct for @ahumesky who is concerned with Android rules maintenance.

I'm mostly interested in Java rules.

[cpsauer]

Sorry about that, @comius--and thanks for taking a peek. Sounds good.
[Confusion was that the others were also all ProGuard/Android motivated.]

Have had the pleasure of interacting with @ahumesky before, and looking forward to talking with him again!

[cpsauer]

Friendly bump :) hoping I can ask you to take a peek when you get a chance

[cpsauer]

Another friendly bump :)

[cpsauer]

@ahumesky, since we'd also interacted on #14966, heads that I think this would be relevant for R8, too, and therefore not obviated.

[ted-xie]

Hi there,

Overall I think this CL looks fine, but I am a little hesitant to approve since it checks in two large files that are autogenerated from Gradle. Unfortunately I don't think there's a substantially better way to do this - either we check in the generated files and continue to maintain them as time goes on, or we engineer the plumbing to actually execute whatever function in ProguardFiles.java generates these configs.

I have two follow-up questions for you:

1. Could you post the actual step-by-step command line(s) you used to generate the files?
2. How often do you think these configs will change?

[cpsauer]

Hey, @ted-xie! Thanks for taking a look. I really appreciate your thoughtful review. You calling it a CL brings me back to my Google days :)

I totally agree on all fronts. Grappled with that a bit myself, but I do think embedding them is probably our best option--and way better than android_binary invoking ProGuard without configuring it for Android, leaving things broken by default. (I had contemplated writing something about that in the PR message, and probably should have, though it sounds like you're probably already on the same page.)

To directly answer your questions:

1. Detailed instructions on the best way I know to regenerate them are at the top of the ProGuard configuration files, so people looking at them would know how to update. The configurations weren't generated from the command line, though, because neither they (nor the Android Gradle Plugin) are included with the command line tools or SDK any longer. Instead they were generated via Android Studio's GUI according to the official instructions linked in the headers I added to those files.
2. I think they'll change every so often but not hugely. The ones I added are different than the very old, deprecated ones that used to be in the deprecated SDK tools package, but not by that much, as I recall. Those were still quite useful compared to not having ProGuard configured for Android, and I think there'll be a good deal of compatibility.

Perhaps we could improve the updating process by adding a comment to the relevant function in ProguardFiles.java that asks people updating it to also update Bazel/Blaze (Blazel!)? Could I maybe ask you to do that if/when we land this, since I presume you have better access to that repo as a Googler?

[As for other options: Embedding the whole Gradle Plugin and invoking it--like we do with ProGuard--seemed worse, since that's just a larger file being copied in and has the same update problem. If there were an easy way to have Bazel remote download and run the latest plugin, then maybe that'd work, but we'd still probably be calling an internal API rather than a documented interface. If it's obvious to you how to do this well, though, you should totally go for it; it just wasn't clear to me that there was a better way forward there.]

All the best,
Chris

[ted-xie]

@cpsauer Haha, accidentally let that bit of Google lingo slip out :)

Re: embedding the gradle plugin into Bazel and running to generate the file: I don't think this is a good idea, since that's essentially trying to embed a whole other build system into Bazel, which is itself a build system. In my mind there should be no coupling between Bazel and Gradle.

I can see about adding the comment to ProguardFiles.java (which comes from AOSP), but in the end it may become a political problem - future AOSP contributors may see the comment and punt it to the Bazel maintainers anyway. It seems likely that the Bazel team will need to learn how to generate this file, which brings us back to my original question #1.

I do see that you've included some instructions and a helpful documentation link for how to generate the files. The problem is that the link to the page may go stale over time, or the contents may change. Furthermore, it seems like you have to do a little bit of magic inside of an existing Android + Gradle project, which is kind of a difficult requirement to enforce for the team that's making Android + Bazel work.

Do you have step-by-step instructions on how to generate the proguard config files from scratch? Ideally that's the type of instructions I'd like to see in the top comments of the config file, so that future maintainers can easily regenerate the files as needed. In my mind, the perfect solution would be to run those steps as part of presubmit and diff the autogenerated config file versus the check-in version so that the configurations never get out-of-date.

[cpsauer]

Hey, @ted-xie :) Thanks for being fun to work with--and responsive.

[Totally agree re not embedding gradle+plugin--just wanted to flag that alternative and make sure we were aligned on not doing it. Sounds like we can close that sub-thread.]

[cpsauer]

To make sure we're not talking past each other on the step-by-step for generating the the ProGuard configurations from Android Studio:

• It's quite easy, and there's seriously no magic.
• It just involves building of an Android Studio/Gradle project with ProGuard enabled--and grabbing the ProGuard configuration they generate.
• In more detail, step-by-step: Just open Android Studio to any app or any of their built-in templates/samples. Turn ProGuard on by adding the Gradle config from the linked official docs (they describe where and how and why very well--better than we can do in plain text). Build for release. And then grab the files from the path in the comment.

I earnestly think that linking to Android's docs is a better idea than copying over more details, and would like to try to convince you. Even if we could be as clear or thorough as they are, duplicating Android's docs in our comments seems more likely to go stale than the official Android docs themselves. Android Studio's mechanisms for generating the files may well change, as they have in the past. (As above, the configurations were once just files included in the sdk rather than generated files.) The Android docs will also improve and change to match, but that's a good thing. When/if Android does change things again, the Bazel person trying to get the latest configuration will want the latest Android instructions to get the latest file, not our old, stale cached instructions. Linking to the Android instructions seems like the highest probability way to equip future Bazelers with up to date instructions for getting the latest ProGuard configurations.

Hence the comments in the file. The goal was, as yours is, to walk future Bazelers through generating newer versions of the files when needed, and I think it's basically parallel to the above. If I've fallen short of what you think we'll need to help future Bazelers generate figure out how to generate the future versions of these files, please help me do better! But I do think we'd better mostly point to the Android docs because I think we'd better not assume that Android Studio will remain the same.

[cpsauer]

[The CI idea is interesting. But I'm not sure quite how we'd do it without creating one of two bigger issues. Do you see a clear way to do it? I think regardless I'd need to ask for some of your help, since I haven't done this from the command line/Android CI before, much less inside of Bazel's CI system.]

[Also, is there a Google-internal way to watch changes to Android source files? If we were doing this inside my org--and the files were on GitHub, we'd just have the code owner for the ProGuard feature sign up for email notifications when the source file in question changed.]

[cpsauer]

Also, curious: How do you all handle this internally? Doesn't it break builds to not have things configured for Android?

[ted-xie]

Thanks for the context @cpsauer! Your comments about the proguard config generation make sense to me. The CI part will be a larger effort; if Android Studio is involved, there may be some CI configuration changes required. We can look into that later.

How do you all handle this internally? Doesn't it break builds to not have things configured for Android?

I was also curious about this and looked into it. The internal toolchains for proguard point to a sh_binary wrapper around proguard that gets invoked with a config file embedded in the action; the config file itself is configurable between several modes (i.e. don't optimize, regular optimization etc) based on a select. The proguard call in Bazel points to a jar in the remote_java_tools repo, which explains why the behavior differs internally vs externally.

I've discussed this PR internally, and I think to merge this we'll at least have to strip out the mandatory() calls in AndroidSdkBaseRule for the new attributes you've added. If these are mandatory, that means every instance of an Android SDK both inside and outside google (i.e. thousands of users) would be affected.

There are a few other open questions about this PR, like how different proguard configs would clash if a user provides their own proguard config to android_library or andorid_binary.

[cpsauer]

Yeah! And thank you @ted-xie!

I really appreciate your looking at the internal one.
Bummer that open source didn't get the goods off the bat! Time to unify the fragmentation--for easier maintenance? [Do you know what's motivating the forking of implementations, here and more generally?]

Doesn't matter to me which way we do it--though there are probably some considerations around overriding ProGuard, windows support, etc. in the background here. It would be really good if the configuration were useable from other rules besides the native android_binary, though, both for the upcoming starlark versions of the rules and for the other use case mentioned in the issue (#14909).

[Finally, re ProGuards clashing: I don't see a problem here--at least not yet. The configurations are idempotent and intended to be accumulated, right? Is there something I'm missing?]

Cheers,
Chris

[ted-xie]

Do you know what's motivating the forking of implementations, here and more generally?

As you alluded to above, there are internal usecases for overriding ProGuard. I'm not sure how "public" those are, so I'll leave it at that.

Thanks for all the extra context!

[cpsauer]

Hmm. Okay! If it's just about overriding ProGuard, then I know (publicly) that R8 support is supposed to be coming (still should need this config). And I think Bazel also allows overriding via --proguard_top.

Meant more to be asking about implementation divergence!

Anyway, that's all at most a side question about this issue. Could I ask where we should be headed next?

[cpsauer]

Hey, @ted-xie, how should we approach solving things from here?
(Just pinging bc it's been a bit.)

[ted-xie]

Hey there,

Sorry for the delayed response! This PR caused a bunch of issues with internal tests:

• Adding new mandatory fields to the SDK declaration was a non-starter; this would impact way too many projects both internal to Google and with open-source Bazel users.
• Marking the new Proguard-related fields as non-mandatory caused some other internal tests to fail. The internal test failures should be reproducible using the Bazel Android integration tests, if the android_sdk stanzas do not have the various proguard fields populated, for example these lines.

In addition to those two issues, the implementation of the default proguard-ing specs differs significantly from the internal implementation which I described earlier. I started mocking up an implementation of this in Bazel but ran out of time due to competing priorities.

[cpsauer]

Hey, Ted! Thanks for replying.

With you on the need for harmony and uniformity with the internal implementation.

I think we all agree that the current external proguard behavior is broken, missing required configuration, which is why you all added code to configure the internal one. I wish that fix made it into Bazel before I took a shot at helping. But seeing as it hasn't, and we'd like harmonious implementations, could I ask you finish porting over Blaze's fix, unifying what should be unified?

[I only wrote this PR because I thought there wasn't already a solution and wanted to take a shot at helping. But I'm not attached to it beyond trying to help, and if you all have an internal implementation you want to use--and only you can see--then it seems like the most efficient way forward is asking for you guys to do the work of reunifying the parts that should be shared with Bazel.]

Thanks,
Chris