Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish Proguard configs with Guava #2117

Closed
cpovirk opened this issue Jul 27, 2015 · 25 comments
Closed

Publish Proguard configs with Guava #2117

cpovirk opened this issue Jul 27, 2015 · 25 comments
Assignees
Labels
P2 package=general status=triaged type=other Miscellaneous activities not covered by other type= labels

Comments

@cpovirk
Copy link
Member

cpovirk commented Jul 27, 2015

We are working on some internally. Once they're ready, we should publish them in Guava. I don't know if there's a standard way to do this with Maven.

@JakeWharton
Copy link

Since Guava is dependency-free, you could publish a sibling artifact using the 'aar' packaging whose format allows embedding proguard rules.

Android users wanting the automatic rules would specify a dependency like

compile 'com.google.guava:guava:19.0@aar'

instead of just

compile 'com.google.guava:guava:19.0'

@JakeWharton
Copy link

(Dependency-free is important because using a package qualifier on a dependency skips the deployed pom.xml for looking up transitive dependencies)

@cpovirk
Copy link
Member Author

cpovirk commented Jul 27, 2015

Thanks. I take it that our <optional>true</optional> dependencies on various projects' annotation artifacts aren't a problem here?

@JakeWharton
Copy link

Right. Any build system parsing the pom.xml should ignore those anyway.

@breyed
Copy link

breyed commented Aug 11, 2016

It would be great if there as a "it just works" solution. It all looks so good at first when a simple

compile 'com.google.guava:guava:19.0'

gives your project all the Guava goodness, but come time to build a real-world APK with

        minifyEnabled true
        shrinkResources true

the fun ends due to all the errors. The error messages are obtuse and don't even provide much clue that Guava's the cause.

@bryanstern
Copy link

Is this dead? The proguard rules in the wiki seem terribly out of date.

@kluever
Copy link
Member

kluever commented Jan 18, 2017

@netdpb any ideas?

@cpovirk
Copy link
Member Author

cpovirk commented Jan 19, 2017

We do have proguard.cfg files internally. We've just never published them :(

@bryanstern
Copy link

@cpovirk Any chance you could start including it in the public repo?

@cpovirk
Copy link
Member Author

cpovirk commented Jan 25, 2017

Shouldn't be too hard. I will make myself a note for when I'm finished with my current project (probably a couple weeks, or maybe I'll need a small project before then).

(We'll still want to do the aar setup eventually, but that will be a bigger project. Or maybe someone can provide a pull request once the files are actually present.)

@bryanstern
Copy link

@cpovirk any updates?

@cpovirk
Copy link
Member Author

cpovirk commented Mar 16, 2017

Ended up needing to take on another project, so I've delayed working on this again, sorry. It's still on my list.

@patrickpilch
Copy link

Any updates?

@gengjiawen
Copy link

Is it possible to change the build tools. If we switch to gradle or bazel like dagger, this will be much easier.

@jbduncan
Copy link
Contributor

jbduncan commented Mar 5, 2018

@gengjiawen I opened an issue for that some time ago: #2850. :)

@ronshapiro
Copy link
Contributor

R8 now understands proguard specs in META-INF/proguard. We could use that to publish without needing to change the packaging

@gengjiawen
Copy link

@ronshapiro Create resource folder in android folder and put the proguard file there ?

@raghsriniv raghsriniv added the P3 label Jun 24, 2019
@raghsriniv raghsriniv added type=other Miscellaneous activities not covered by other type= labels and removed type=dev labels Aug 5, 2019
@icbaker
Copy link

icbaker commented Sep 15, 2020

I was wondering if there was any follow-up to @ronshapiro's comment about including the config even inside a jar? I didn't see anything in the 27.1-android jar.

Is the recommended solution to copy-paste the contents of https://github.com/google/guava/wiki/UsingProGuardWithGuava into our own library/app proguard config?

@cpovirk
Copy link
Member Author

cpovirk commented Sep 15, 2020

No followup. This is another thing that "someone should do." And it's another thing that is becoming only more important over time. Hopefully that will help us make it happen.

@cpovirk
Copy link
Member Author

cpovirk commented Oct 20, 2020

Truth may also benefit from one of these. It looks like all we have internally is -dontwarn java.lang.SafeVarargs, but we may want to do more around our reflective access to ActualValueInference/ASM (touched recently in google/truth@aea78e8 and google/truth@0bfa285).

copybara-service bot pushed a commit that referenced this issue Apr 6, 2023
…tations when they're available.

They're never available under an Android VM, but we also test guava-android under a JRE (which is always Java 8+ now, which makes our implementation job a bit easier), so the annotations _are_ available there.

This is a step toward letting us [use type-use annotations more widely in Guava](jspecify/jspecify#239). In particular, it unblocks cl/521011962 (which in turn reduces the number of merge conflicts we'll see when using type-use annotations in our tests), and it helps our other testing of type-use annotations in Guava.

In order to implement this, I copied two methods that were previously only available in guava-jre to make them also available in guava-android for `NullPointerTester` there: `Invokable.getAnnotatedReturnType()` and `Parameter.getAnnotatedType()`. Those methods do not work under and Android VM (where, again, type-use annotations aren't available), but they're useful for running our tests under a JRE.

FYI: We did see one project that needed some new `-dontwarn` lines in [our Proguard config](#2117) as a result of this CL. But we expect this to be unlikely, especially since reflection is typically a bad idea under Android. (An alternative approach would be for us to expose two _new_ methods that are similar to `getAnnotatedReturnType()` and `getAnnotatedType()` but that declare their return type as plain `Object`. That would be safer, but I think there's actually more value in testing the more aggressive approach: We'd like to someday be more aggressive about adding APIs that use Java 8+ types in guava-android, so it's nice to test them on low-stakes `@Beta` APIs that shouldn't be used from Android to begin with.)

On the plus side, we no longer need to perform reflection on account of j2objc: As planned in cl/367051816, j2objc was subsequently updated in cl/367072772 to contain stub implementations of the necessary `TypeVariable` methods.

RELNOTES=n/a
PiperOrigin-RevId: 521795608
copybara-service bot pushed a commit that referenced this issue Apr 6, 2023
…tations when they're available.

They're never available under an Android VM, but we also test guava-android under a JRE (which is always Java 8+ now, which makes our implementation job a bit easier), so the annotations _are_ available there.

This is a step toward letting us [use type-use annotations more widely in Guava](jspecify/jspecify#239). In particular, it unblocks cl/521011962 (which in turn reduces the number of merge conflicts we'll see when using type-use annotations in our tests), and it helps our other testing of type-use annotations in Guava.

In order to implement this, I copied two methods that were previously only available in guava-jre to make them also available in guava-android for `NullPointerTester` there: `Invokable.getAnnotatedReturnType()` and `Parameter.getAnnotatedType()`. Those methods do not work under and Android VM (where, again, type-use annotations aren't available), but they're useful for running our tests under a JRE.

FYI: We did see one project that needed some new `-dontwarn` lines in [our Proguard config](#2117) as a result of this CL. But we expect this to be unlikely, especially since reflection is typically a bad idea under Android. (An alternative approach would be for us to expose two _new_ methods that are similar to `getAnnotatedReturnType()` and `getAnnotatedType()` but that declare their return type as plain `Object`. That would be safer, but I think there's actually more value in testing the more aggressive approach: We'd like to someday be more aggressive about adding APIs that use Java 8+ types in guava-android, so it's nice to test them on low-stakes `@Beta` APIs that shouldn't be used from Android to begin with.)

On the plus side, we no longer need to perform reflection on account of j2objc: As planned in cl/367051816, j2objc was subsequently updated in cl/367072772 to contain stub implementations of the necessary `TypeVariable` methods.

RELNOTES=n/a
PiperOrigin-RevId: 521795608
copybara-service bot pushed a commit that referenced this issue Apr 6, 2023
…tations when they're available.

They're never available under an Android VM, but we also test guava-android under a JRE (which is always Java 8+ now, which makes our implementation job a bit easier), so the annotations _are_ available there.

This is a step toward letting us [use type-use annotations more widely in Guava](jspecify/jspecify#239). In particular, it unblocks cl/521011962 (which in turn reduces the number of merge conflicts we'll see when using type-use annotations in our tests), and it helps our other testing of type-use annotations in Guava.

In order to implement this, I copied two methods that were previously only available in guava-jre to make them also available in guava-android for `NullPointerTester` there: `Invokable.getAnnotatedReturnType()` and `Parameter.getAnnotatedType()`. Those methods do not work under and Android VM (where, again, type-use annotations aren't available), but they're useful for running our tests under a JRE.

FYI: We did see one project that needed some new `-dontwarn` lines in [our Proguard config](#2117) as a result of this CL. But we expect this to be unlikely, especially since reflection is typically a bad idea under Android. (An alternative approach would be for us to expose two _new_ methods that are similar to `getAnnotatedReturnType()` and `getAnnotatedType()` but that declare their return type as plain `Object`. That would be safer, but I think there's actually more value in testing the more aggressive approach: We'd like to someday be more aggressive about adding APIs that use Java 8+ types in guava-android, so it's nice to test them on low-stakes `@Beta` APIs that shouldn't be used from Android to begin with.)

On the plus side, we no longer need to perform reflection on account of j2objc: As planned in cl/367051816, j2objc was subsequently updated in cl/367072772 to contain stub implementations of the necessary `TypeVariable` methods.

RELNOTES=n/a
PiperOrigin-RevId: 521795608
copybara-service bot pushed a commit that referenced this issue Apr 6, 2023
…tations when they're available.

They're never available under an Android VM, but we also test guava-android under a JRE (which is always Java 8+ now, which makes our implementation job a bit easier), so the annotations _are_ available there.

This is a step toward letting us [use type-use annotations more widely in Guava](jspecify/jspecify#239). In particular, it unblocks cl/521011962 (which in turn reduces the number of merge conflicts we'll see when using type-use annotations in our tests), and it helps our other testing of type-use annotations in Guava.

In order to implement this, I copied two methods that were previously only available in guava-jre to make them also available in guava-android for `NullPointerTester` there: `Invokable.getAnnotatedReturnType()` and `Parameter.getAnnotatedType()`. Those methods do not work under and Android VM (where, again, type-use annotations aren't available), but they're useful for running our tests under a JRE.

FYI: We did see one project that needed some new `-dontwarn` lines in [our Proguard config](#2117) as a result of this CL. But we expect this to be unlikely, especially since reflection is typically a bad idea under Android. (An alternative approach would be for us to expose two _new_ methods that are similar to `getAnnotatedReturnType()` and `getAnnotatedType()` but that declare their return type as plain `Object`. That would be safer, but I think there's actually more value in testing the more aggressive approach: We'd like to someday be more aggressive about adding APIs that use Java 8+ types in guava-android, so it's nice to test them on low-stakes `@Beta` APIs that shouldn't be used from Android to begin with.)

On the plus side, we no longer need to perform reflection on account of j2objc: As planned in cl/367051816, j2objc was subsequently updated in cl/367072772 to contain stub implementations of the necessary `TypeVariable` methods.

RELNOTES=n/a
PiperOrigin-RevId: 522214387
@lwasyl
Copy link

lwasyl commented May 5, 2023

Please be aware that starting with Android Gradle Plugin 8.x, R8 doesn't add -ignorewarnings option automatically. So now a build with default configuration starts failing on if rules from https://github.com/google/guava/wiki/UsingProGuardWithGuava are not applied. Sample error:

ERROR: R8: Missing class java.lang.ClassValue (referenced from: java.lang.ClassValue com.google.common.util.concurrent.FuturesGetChecked$GetCheckedTypeValidatorHolder$ClassValueValidator.isValidClass and 3 other contexts)

This requires the developers to consider the warning and add corresponding rules manually. Since Guava is often a transitive dependency (via Dagger or AndroidX libraries) and not direct one, the https://github.com/google/guava/wiki/UsingProGuardWithGuava is not very discoverable (in fact I only found it after googling the error and going through issuetracker to this issue)

@cpovirk
Copy link
Member Author

cpovirk commented May 5, 2023

Thank you, that's good to know about.

The ClassValue usage specifically was removed for 31.0 (released about a year and a half ago), so I'm wondering how much it helps to upgrade the version of Guava you're using (which ideally is something that Dagger and AndroidX would do, too). My guess would be that there are other warnings (corresponding to the other -dontwarn lines in the rules in the wiki), but can you confirm?

(The other (non--dontwarn) rules in the wiki are useful for people who are actually using optimization or minimization, but I wouldn't expect them to be required to avoid build errors.)

(I wonder if we could avoid some of those errors more directly by removing references, like to App Engine, which is of course not necessary for Android users and which might not be necessary even for any App Engine users of guava-android nowadays.)

Extra bonus points if someone can point us at a trivial project that demonstrates these failures, since otherwise it will be hard to be sure that we've solved them.

@icbaker
Copy link

icbaker commented May 5, 2023

I believe (could be wrong) that androidx.media3 is the only AndroidX library that depends on 'full' Guava (the others only depend on com.google.guava:listenablefuture). Media3 has upgraded to Guava 31.0.1 and this will be included in the 1.1.0 release.

@cpovirk
Copy link
Member Author

cpovirk commented May 8, 2023

The META-INF approach is documented here and demonstrated by https://repo1.maven.org/maven2/com/squareup/okhttp3/okhttp/4.11.0/okhttp-4.11.0.jar.

I think we can make that happen, but I'm not sure I'll get to it this week.

@cpovirk cpovirk added P2 and removed P3 labels May 8, 2023
copybara-service bot pushed a commit that referenced this issue May 8, 2023
Fixes #6468

I'd looked at [how Guice does this](https://github.com/google/guice/blob/40a5bcfab5cfe45c3b6c5ffc9309b310df82775b/pom.xml#L255-L273), which pulls the Apache license from a remote server (and also generates additional `DEPENDENCIES` and `NOTICE` files). But I figured it was simplest to just point `maven-resources-plugin` at our local `LICENSE` file: `maven-resources-plugin` can work with any license, whereas I'm not sure if Guice's `apache-jar-resource-bundle` approach can. Plus, I want to figure `maven-resources-plugin` out, anyway, for [Proguard purposes](#2117).

Conveniently, I'm pretty confident that our existing `resources` configuration isn't necessary: Maybe it was years ago, but nowadays, we have no results for `find */src android/*/src -type f -not -name '*.java'` (except under `guava-gwt`, which overrides `resources` and which I verified builds an identical jar after this change).

RELNOTES=n/a
PiperOrigin-RevId: 530338363
copybara-service bot pushed a commit that referenced this issue May 8, 2023
Fixes #6468

I'd looked at [how Guice does this](https://github.com/google/guice/blob/40a5bcfab5cfe45c3b6c5ffc9309b310df82775b/pom.xml#L255-L273), which pulls the Apache license from a remote server (and also generates additional `DEPENDENCIES` and `NOTICE` files). But I figured it was simplest to just point `maven-resources-plugin` at our local `LICENSE` file: `maven-resources-plugin` can work with any license, whereas I'm not sure if Guice's `apache-jar-resource-bundle` approach can. Plus, I want to figure `maven-resources-plugin` out, anyway, for [Proguard purposes](#2117).

Conveniently, I'm pretty confident that our existing `resources` configuration isn't necessary: Maybe it was years ago, but nowadays, we have no results for `find */src android/*/src -type f -not -name '*.java'` (except under `guava-gwt`, which overrides `resources` and which I verified builds an identical jar after this change).

RELNOTES=n/a
PiperOrigin-RevId: 530360631
copybara-service bot pushed a commit that referenced this issue May 9, 2023
Fixes #2117

RELNOTES=The published Guava jar now includes Proguard configurations that are picked up automatically by the Android Gradle Plugin. This should help with warnings that were promoted to errors in Android Gradle Plugin 8.x.
PiperOrigin-RevId: 530327051
@cpovirk
Copy link
Member Author

cpovirk commented May 9, 2023

Truth may also benefit from one of these. It looks like all we have internally is -dontwarn java.lang.SafeVarargs

SafeVarargs is covered by the Proguard configs that will be published with the next Guava release, and of course Guava is a dependency of Truth, so there's nothing we'd need to do for Truth for that.

(Plus, SafeVarargs is part of android.jar nowadays, so likely even the Guava entries are redundant.)

, but we may want to do more around our reflective access to ActualValueInference/ASM (touched recently in google/truth@aea78e8 and google/truth@0bfa285).

That might still end up being necessary, but I'll wait to hear of trouble before looking into it (in part because I'll find it hard to be confident in a fix until I can reproduce the problem :)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general status=triaged type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.