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

Add Java8+ APIs to guava-android #6567

Open
swankjesse opened this issue Jun 20, 2023 · 10 comments
Open

Add Java8+ APIs to guava-android #6567

swankjesse opened this issue Jun 20, 2023 · 10 comments
Assignees
Labels
P3 package=general type=enhancement Make an existing feature better

Comments

@swankjesse
Copy link

When we upgraded Truth we started getting crashes like this:

java.lang.NoSuchMethodError: 'java.util.stream.Collector com.google.common.collect.Sets.toImmutableEnumSet()'
	at com.android.resources.ResourceType.<clinit>(ResourceType.java:175)
	at app.cash.paparazzi.internal.DynamicResourceIdManager$IdProvider.<init>(DynamicResourceIdManager.kt:14)
	at app.cash.paparazzi.internal.DynamicResourceIdManager.<init>(DynamicResourceIdManager.kt:28)
	at app.cash.paparazzi.internal.PaparazziCallback.<init>(PaparazziCallback.kt:52)
	at app.cash.paparazzi.Paparazzi.prepare(Paparazzi.kt:145)
	at app.cash.paparazzi.Paparazzi$apply$statement$1.evaluate(Paparazzi.kt:116)

As @TWiStErRob explains on Paparazzi #906:

This came back in Truth 1.1.5 because Paparazzi depends on 32.0.0-jre vs Truth uses 32.0.1-android, and the higher version number wins. Looks like the workaround has to stay.

A very simple fix for our problem is for Guava to support the toImmutableEnumSet() function on all platforms. I don’t believe this function needs the same implementation on all platforms; it just needs something so we don’t crash at runtime.

@cpovirk
Copy link
Member

cpovirk commented Jun 21, 2023

Sorry, this kind of problem crops up occasionally. See, e.g., https://stackoverflow.com/q/65646026/28465.

Since we dropped support for Java 7 back in 31.0, we can in theory now get away with adding methods that use Java 8+ types like Collector in their signatures. That's true even if Android users don't use library desugaring. (And I'm not sure we could require them to use it yet. Issue 203113147 would help but probably not be sufficient).) Thankfully, we can rely on the Android VM to ignore those methods unless someone actually calls them. (And maybe we can use @RequiresApi to prevent people from calling them by accident. But that may annoy people who target older versions of Android but use library desugaring (since Android Lint would still report an error for calls to the method, even though it's actually safe for them). And maybe the androidx.annotation:annotation-jvm dependency would become another obstacle to modularization? But this is becoming a long digression.)

Can we get away with it in practice? I'm going to run a test with toImmutableEnumSet() specifically to see what happens. I do know that we've seen trouble when testing another Java 8+ thing—specifically, adding one default method, which I might have guessed would be easier than adding methods that use Java 8+ APIs. But adding another default method appeared to work fine.... We're never going to feel completely safe with making these kinds of changes, but we'll have to take the plunge eventually. We took a baby step in this direction by adding a usage of a class that isn't support on Android at all in 32.0.0 (namely, a usage of AnnotatedType). That seems to have gone OK so far.

I do note that this general class of problem will exist as long as guava-jre contains any APIs that guava-android doesn't. And I'm unsure whether the two will fully converge. But this problem is a reason to consider making them converge, even if it doesn't make fully sense. And even if they don't fully converge, there's a big difference between missing commonly used APIs (like our Collector implementations) and missing only the more obscure things.

Independent of all that, one thing that we really, really should do is finally get back to #3683: It is designed to make Gradle pick the maximum numeric part of the version number for Guava but then choose -android or -jre as appropriate. That PR is frequently on my mind; I just need to find an hour on a day when I'm feeling up for a deep dive and when nothing else is on fire :)

(For non-Gradle users, we could additionally consider releasing separate truth-android and truth-jre artifacts, which would have identical class files but would have a different Guava dep. (I wonder if any tools had to go to effort to specifically recognize Guava's unusual version numbers, and I wonder if a similar change in Truth would cause them similar issues. And I wonder if the Truth change would "look bad" even if it turned out to be a net win in fighting the Guava problem.... For anyone else at Google following along, see bug 177249370.)

@cpovirk
Copy link
Member

cpovirk commented Jun 22, 2023

It looks like we'd be able to add toImmutableEnumSet() to guava-android without breaking anything inside Google.

I'm thinking I might try something like this:

  • Pick a less commonly used class (ImmutableTable?) whose Java 8+ methods we could add to guava-android. (Bonus: The relevant ImmutableTable methods return a Collector, so users are unlikely to call the methods unless they're also using other Java 8+ APIs. This means that we can probably count on existing NewApi enforcement without having to use @RequiresApi ourselves. And really, the Collector APIs are probably a sizable portion of what users want, so it's good to try adding one of them.)
  • Try actually submitting that internally and seeing it get used.
  • Consider repeating that for other APIs.
  • Release the first such API publicly.
  • If all goes well, do more of that, thinking more about @RequiresApi.

I don't expect to add any of the APIs in the next Guava release. But that's only because we've had a surprising number of changes that I'd like to release soonish so that users can pick them out without taking even a small risk from ImmutableTable.toImmutableTable (or whatever we end up adding). But we won't need to wait a year before releasing again :)

(I'm going to generalize this issue to be about the overall effort.)

@cpovirk cpovirk changed the title Support Sets.toImmutableEnumSet() on Android Add Java8+ APIs to guava-android Jun 22, 2023
@cpovirk cpovirk self-assigned this Jun 22, 2023
@cpovirk cpovirk added type=enhancement Make an existing feature better package=general P3 labels Jun 22, 2023
@cpovirk
Copy link
Member

cpovirk commented Jun 22, 2023

Actually, we added Stopwatch.elapsed() (which returns a Java 8 Duration) to our internal Android copy a while back, but I'd forgotten. It's gone well. But I'd note that we added special support to our Java compatibility enforcement (in this change to AndroidJdkLibsChecker), so we might want similar enforcement for future changes (albeit perhaps not for Collector APIs, given what I said above).

copybara-service bot pushed a commit that referenced this issue Jun 26, 2023
…-android` (but don't do so yet).

Progress toward #6567

RELNOTES=n/a
PiperOrigin-RevId: 542547311
copybara-service bot pushed a commit that referenced this issue Jun 26, 2023
…-android` (but don't do so yet).

Progress toward #6567

RELNOTES=n/a
PiperOrigin-RevId: 543468006
copybara-service bot pushed a commit that referenced this issue Jun 28, 2023
To be conservative, this commit does not removes most annotation artifacts from Gradle's runtime classpath, only j2objc-annotations. The other artifacts contain at least some annotations with RUNTIME visibility (IIRC). (Even this change could theoretically affect users who assume that they can read CLASS-retention annotations (of which j2objc-annotations has some) from bytecode and find them in the runtime classpath. But that seems unlikely, especially for j2objc annotations.) We may consider being more aggressive in the future. For now, this particular commit addresses #2824 for Gradle users just a tiny bit, and it helps with the problem that prompted #6567.

Fixes #3683

RELNOTES=Added [Gradle Module Metadata](https://docs.gradle.org/current/userguide/publishing_gradle_module_metadata.html). If you use Gradle 6 or higher, Gradle can automatically intelligently resolve conflicts between `guava-android` and `guava-jre`, among [other benefits](#3683).
PiperOrigin-RevId: 337348979
copybara-service bot pushed a commit that referenced this issue Jun 28, 2023
To be conservative, this commit does not removes most annotation artifacts from Gradle's runtime classpath, only j2objc-annotations. The other artifacts contain at least some annotations with RUNTIME visibility (IIRC). (Even this change could theoretically affect users who assume that they can read CLASS-retention annotations (of which j2objc-annotations has some) from bytecode and find them in the runtime classpath. But that seems unlikely, especially for j2objc annotations.) We may consider being more aggressive in the future. For now, this particular commit addresses #2824 for Gradle users just a tiny bit, and it helps with the problem that prompted #6567.

Fixes #3683

RELNOTES=Added [Gradle Module Metadata](https://docs.gradle.org/current/userguide/publishing_gradle_module_metadata.html). If you use Gradle 6 or higher, Gradle can automatically intelligently resolve conflicts between `guava-android` and `guava-jre`, among [other benefits](#3683).
PiperOrigin-RevId: 544108700
@TWiStErRob
Copy link

@swankjesse, from my reading the original issue will be solved by ce78fc6, see the section titled "Selecting the appropriate flavor" in 32.1 release notes, will test when 32.1.1 is released.

@cpovirk
Copy link
Member

cpovirk commented Jun 30, 2023

Thanks, yes, that's the theory. I've released 32.1.1. Sorry for the trouble with 32.1.0. Please do let us know how it goes.

@TWiStErRob
Copy link

Not trivial... got a capability conflict, will have to look deeper later.

copybara-service bot pushed a commit that referenced this issue Jul 25, 2023
The primary change comes as a followup to cl/543468006: Now that the Google-internal `ImmutableTable` declares [methods with Java 8+ types in their signatures](#6567), we get a `NoClassDefFoundError` when performing reflection on the class under old Android emulators. We perform reflection only when (a) using `NullPointerTester` or (b) using serialization. So we (a) stop running `NullPointerTester` under an Android emulator (though we continue to run the same `guava-android` test under a JVM) and (b) specify a `serialVersionUID` so that serialization doesn't need to compute one. (Specifying `serialVersionUID` was our MO for a while. Perhaps we just got careless, and perhaps we should be more diligent. Alternatively, perhaps we should change the `serialVersionUID` every release or so so as to encourage people [not to persist serialized data](https://github.com/google/guava#important-warnings). I at least went to a tiny bit of effort to keep `guava-jre` and `guava-android` on different `serialVersionUID` values, though maybe that's more likely to cause needless trouble someday than to prevent it?)

The other change is to avoid running another GC test under an Android emulator. We'd originally done this for one copy of `AbstractIteratorTester` in cl/444929745, but for whatever reason, we didn't need it for the other copy until (I assume) cl/546307327 and its switch to a different emulator version.

RELNOTES=n/a
PiperOrigin-RevId: 550670580
copybara-service bot pushed a commit that referenced this issue Jul 25, 2023
The primary change comes as a followup to cl/543468006: Now that the Google-internal `ImmutableTable` declares [methods with Java 8+ types in their signatures](#6567), we get a `NoClassDefFoundError` when performing reflection on the class under old Android emulators. We perform reflection only when (a) using `NullPointerTester` or (b) using serialization. So we (a) stop running `NullPointerTester` under an Android emulator (though we continue to run the same `guava-android` test under a JVM) and (b) specify a `serialVersionUID` so that serialization doesn't need to compute one. (Specifying `serialVersionUID` was our MO for a while. Perhaps we just got careless, and perhaps we should be more diligent. Alternatively, perhaps we should change the `serialVersionUID` every release or so so as to encourage people [not to persist serialized data](https://github.com/google/guava#important-warnings). I at least went to a tiny bit of effort to keep `guava-jre` and `guava-android` on different `serialVersionUID` values, though maybe that's more likely to cause needless trouble someday than to prevent it?)

The other change is to avoid running another GC test under an Android emulator. We'd originally done this for one copy of `AbstractIteratorTester` in cl/444929745, but for whatever reason, we didn't need it for the other copy until (I assume) cl/546307327 and its switch to a different emulator version.

RELNOTES=n/a
PiperOrigin-RevId: 550872250
copybara-service bot pushed a commit that referenced this issue Oct 21, 2023
…/docs/api/java.base/java/util/SequencedCollection.html) methods to `ImmutableSortedSet`.

There are a couple reasons that this is maybe kind of nice, and then there's my real motivation for doing it.

Reasons that this is maybe kind of nice:

- We add `@DoNotCall` and `@Deprecated` to the mutator methods, as usual.
- We refine the return type of `reversed()` to `ImmutableSortedSet`.

But again, those aren't my real motivation. (If they _were_ my real motivation, you might notice, then there would be no reason for us to override `getFirst()` and `getLast()`.)

My _real_ motivation is that we are looking to give `guava-android` APIs that use Java 8+ types in their signatures, such as `ImmutableSet.toImmutableSet`. And as a result of some part of those changes, we start seeing errors in some of our Google-internal build rules. Those errors come because some the desugaring process is inserting its own versions of `getFirst` and friends, versions that refer to the desugar-provided copies of `SortedSet` and `NavigableSet`, which our build process then looks for but cannot find.

Since this CL is preparation for adding `ImmutableSet.toImmutableSet`, it constitutes further progress toward #6567

RELNOTES=`collect`: Added [`SequencedCollection`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/SequencedCollection.html) methods to `ImmutableSortedSet`.
PiperOrigin-RevId: 575207552
copybara-service bot pushed a commit that referenced this issue Oct 21, 2023
…/docs/api/java.base/java/util/SequencedCollection.html) methods to `ImmutableSortedSet`.

There are a couple reasons that this is maybe kind of nice, and then there's my real motivation for doing it.

Reasons that this is maybe kind of nice:

- We add `@DoNotCall` and `@Deprecated` to the mutator methods, as usual.
- We refine the return type of `reversed()` to `ImmutableSortedSet`.

But again, those aren't my real motivation. (If they _were_ my real motivation, you might notice, then there would be no reason for us to override `getFirst()` and `getLast()`.)

My _real_ motivation is that we are looking to give `guava-android` APIs that use Java 8+ types in their signatures, such as `ImmutableSet.toImmutableSet`. And as a result of some part of those changes, we start seeing errors in some of our Google-internal build rules. Those errors come because some the desugaring process is inserting its own versions of `getFirst` and friends, versions that refer to the desugar-provided copies of `SortedSet` and `NavigableSet`, which our build process then looks for but cannot find.

Since this CL is preparation for adding `ImmutableSet.toImmutableSet`, it constitutes further progress toward #6567

RELNOTES=`collect`: Added [`SequencedCollection`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/SequencedCollection.html) methods to `ImmutableSortedSet`.
PiperOrigin-RevId: 575459262
copybara-service bot pushed a commit that referenced this issue Apr 21, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 21, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 21, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 21, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 22, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 22, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 626483101
copybara-service bot pushed a commit that referenced this issue Apr 22, 2024
…package-private method.

(The latter prepares for [Android compatibility for Java 8 APIs](#6567).)

RELNOTES=n/a
PiperOrigin-RevId: 627098517
copybara-service bot pushed a commit that referenced this issue Apr 29, 2024
(for example, `ImmutableList.toImmutableList()`)

For now, we're making them `@Beta` just in case users encounter enough problems that we find it less disruptive to revert this change than to keep it. However, we plan to remove `@Beta` soon, at which point we'll be committed to this APIs. If you use Guava under Android, please [test with Guava 33.0.0 or higher](https://groups.google.com/g/guava-announce/c/9-dw_C6G_NM), ideally with 34.0.0 or higher (which will contain this commit), and [report any problems](https://github.com/google/guava/issues/new?assignees=&labels=type%3Ddefect&projects=&template=bug_report.yaml).

Our expectation is that this commit should not cause problems, even for users who don't use enable [library desugaring](https://developer.android.com/studio/write/java11-default-support-table). But we will see what happens in wild.

Of course, if you want to actually _use_ these APIs, then you'll need to [enable library desugaring](https://developer.android.com/studio/write/java8-support#library-desugaring) or target a [new enough version of Android](https://developer.android.com/reference/java/util/stream/Stream), just as with any other `Stream`-based APIs.

(progress toward #6567)

RELNOTES=`collect`: Made our `Collector` APIs (e.g., `ImmutableList.toImmutableList()`) available in `guava-android`.
PiperOrigin-RevId: 629191336
copybara-service bot pushed a commit that referenced this issue Apr 30, 2024
(for example, `ImmutableList.toImmutableList()`)

For now, we're making them `@Beta` just in case users encounter enough problems that we find it less disruptive to revert this change than to keep it. However, we plan to remove `@Beta` soon, at which point we'll be committed to this APIs. If you use Guava under Android, please [test with Guava 33.0.0 or higher](https://groups.google.com/g/guava-announce/c/9-dw_C6G_NM), ideally with 34.0.0 or higher (which will contain this commit), and [report any problems](https://github.com/google/guava/issues/new?assignees=&labels=type%3Ddefect&projects=&template=bug_report.yaml).

Our expectation is that this commit should not cause problems, even for users who don't use enable [library desugaring](https://developer.android.com/studio/write/java11-default-support-table). But we will see what happens in wild.

Of course, if you want to actually _use_ these APIs, then you'll need to [enable library desugaring](https://developer.android.com/studio/write/java8-support#library-desugaring) or target a [new enough version of Android](https://developer.android.com/reference/java/util/stream/Stream), just as with any other `Stream`-based APIs.

(progress toward #6567)

RELNOTES=`collect`: Made our `Collector` APIs (e.g., `ImmutableList.toImmutableList()`) available in `guava-android`.
PiperOrigin-RevId: 629191336
copybara-service bot pushed a commit that referenced this issue Apr 30, 2024
(for example, `ImmutableList.toImmutableList()`)

For now, we're making them `@Beta` just in case users encounter enough problems that we find it less disruptive to revert this change than to keep it. However, we plan to remove `@Beta` soon, at which point we'll be committed to this APIs. If you use Guava under Android, please [test with Guava 33.0.0 or higher](https://groups.google.com/g/guava-announce/c/9-dw_C6G_NM), ideally with 34.0.0 or higher (which will contain this commit), and [report any problems](https://github.com/google/guava/issues/new?assignees=&labels=type%3Ddefect&projects=&template=bug_report.yaml).

Our expectation is that this commit should not cause problems, even for users who don't use enable [library desugaring](https://developer.android.com/studio/write/java11-default-support-table). But we will see what happens in wild.

Of course, if you want to actually _use_ these APIs, then you'll need to [enable library desugaring](https://developer.android.com/studio/write/java8-support#library-desugaring) or target a [new enough version of Android](https://developer.android.com/reference/java/util/stream/Stream), just as with any other `Stream`-based APIs.

(progress toward #6567)

RELNOTES=`collect`: Made our `Collector` APIs (e.g., `ImmutableList.toImmutableList()`) available in `guava-android`.
PiperOrigin-RevId: 629491350
@cpovirk
Copy link
Member

cpovirk commented May 2, 2024

33.2.0 adds Collector APIs, including Sets.immutableEnumSet, to guava-android.

Barring surprises, we'll add most Java 8+ APIs in the next release or thereafter.

copybara-service bot pushed a commit that referenced this issue May 17, 2024
…testing.

We had disabled the tests in that environment because `NullPointerTester` performed reflection on types that contained `Collector`-based APIs. Since `Collector` wasn't available, the tests failed. But now, we've enabled library desugaring for our internal tests (though we continue not to _require_ library desugaring unless users actually use such APIs), so the tests can work again.

(relevant to #6567)

RELNOTES=n/a
PiperOrigin-RevId: 634770528
copybara-service bot pushed a commit that referenced this issue May 17, 2024
…testing.

We had disabled the tests in that environment because `NullPointerTester` performed reflection on types that contained `Collector`-based APIs. Since `Collector` wasn't available, the tests failed. But now, we've enabled library desugaring for our internal tests (though we continue not to _require_ library desugaring unless users actually use such APIs), so the tests can work again.

(relevant to #6567)

RELNOTES=n/a
PiperOrigin-RevId: 634784425
copybara-service bot pushed a commit that referenced this issue Jun 2, 2024
(progress toward #6567)

RELNOTES=`collect`: Removed `@Beta` from the `guava-android` `Collector` APIs.
PiperOrigin-RevId: 630165794
copybara-service bot pushed a commit that referenced this issue Jun 2, 2024
(progress toward #6567)

RELNOTES=`collect`: Removed `@Beta` from the `guava-android` `Collector` APIs.
PiperOrigin-RevId: 630165794
copybara-service bot pushed a commit that referenced this issue Jun 2, 2024
(progress toward #6567)

RELNOTES=`collect`: Removed `@Beta` from the `guava-android` `Collector` APIs.
PiperOrigin-RevId: 639518116
copybara-service bot pushed a commit that referenced this issue Jun 13, 2024
Also, related minor changes:
- Import `java.time.Duration` instead of using it fully qualified. (Somehow, existing unqualified usages of `Duration` in _Javadoc_ references were working even without the import.) Compare cl/641315337.
- "Upstream" a fix to a Javadoc reference to `maximumSize` from the Android flavor to the mainline.

Fixes #7232
Progress on #6567

RELNOTES=`cache`: Added `CacheBuilder` `Duration` overloads to `guava-android`.
PiperOrigin-RevId: 642709673
copybara-service bot pushed a commit that referenced this issue Jun 13, 2024
Also, related minor changes:
- Import `java.time.Duration` instead of using it fully qualified. (Somehow, existing unqualified usages of `Duration` in _Javadoc_ references were working even without the import.) Compare cl/641315337.
- "Upstream" a fix to a Javadoc reference to `maximumSize` from the Android flavor to the mainline.

Fixes #7232
Progress on #6567

RELNOTES=`cache`: Added `CacheBuilder` `Duration` overloads to `guava-android`.
PiperOrigin-RevId: 642709673
copybara-service bot pushed a commit that referenced this issue Jun 13, 2024
Also, related minor changes:
- Import `java.time.Duration` instead of using it fully qualified. (Somehow, existing unqualified usages of `Duration` in _Javadoc_ references were working even without the import.) Compare cl/641315337.
- "Upstream" a fix to a Javadoc reference to `maximumSize` from the Android flavor to the mainline.

Fixes #7232
Progress on #6567

RELNOTES=`cache`: Added `CacheBuilder` `Duration` overloads to `guava-android`.
PiperOrigin-RevId: 642709673
copybara-service bot pushed a commit that referenced this issue Jun 13, 2024
Also, related minor changes:
- Import `java.time.Duration` instead of using it fully qualified. (Somehow, existing unqualified usages of `Duration` in _Javadoc_ references were working even without the import.) Compare cl/641315337.
- "Upstream" a fix to a Javadoc reference to `maximumSize` from the Android flavor to the mainline.

Fixes #7232
Progress on #6567

RELNOTES=`cache`: Added `CacheBuilder` `Duration` overloads to `guava-android`.
PiperOrigin-RevId: 642709673
copybara-service bot pushed a commit that referenced this issue Jun 13, 2024
Also, related minor changes:
- Import `java.time.Duration` instead of using it fully qualified. (Somehow, existing unqualified usages of `Duration` in _Javadoc_ references were working even without the import.) Compare cl/641315337.
- "Upstream" a fix to a Javadoc reference to `maximumSize` from the Android flavor to the mainline.

Fixes #7232
Progress on #6567

RELNOTES=`cache`: Added `CacheBuilder` `Duration` overloads to `guava-android`.
PiperOrigin-RevId: 642993916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=general type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

4 participants
@swankjesse @cpovirk @TWiStErRob and others