google / guava

Google core libraries for Java
Apache License 2.0
50.16k stars 10.9k forks source link

30.1-android triggers Java 8 desugaring checks #5358

Closed ejona86 closed 3 years ago

ejona86 commented 3 years ago

I'm seeing a compilation failure with 30.1-android. I don't see it with 30.0-android.

Execution failed for task ':grpc-android-interop-testing:mergeDexDebug'.
> Could not resolve all files for configuration ':grpc-android-interop-testing:debugRuntimeClasspath'.
   > Failed to transform guava-30.1-android.jar (com.google.guava:guava:30.1-android) to match attributes {artifactType=android-dex, dexing-enable-desugaring=false, dexing-is-debuggable=true, dexing-min-sdk=16, org.gradle.category=library, org.gradle.libraryelements=jar, org.gradle.status=release, org.gradle.usage=java-runtime}.
      > Execution failed for DexingNoClasspathTransform: /home/ejona/.gradle/caches/modules-2/files-2.1/com.google.guava/guava/30.1-android/1e904cca24cdf3dc84d5fe4f73e12f1186e04380/guava-30.1-android.jar.
         > Error while dexing.
           The dependency contains Java 8 bytecode. Please enable desugaring by adding the following to build.gradle
           android {
               compileOptions {
                   sourceCompatibility 1.8
                   targetCompatibility 1.8
               }
           }
           See https://developer.android.com/studio/write/java8-support.html for details. Alternatively, increase the minSdkVersion to 26 or above.

https://github.com/google/guava/commit/dc52e6e385fc9f0571dbd046d8ce5bf1a1c1f5cd looks suspicious. It seems the code expected the build to ignore the Java 8 bytecode and fail at runtime. A minute of glancing didn't show a way to ignore the error without enabling desugaring.

The release notes didn't mention this new requirement. Is this an accident or on purpose? Clearly when Java 7 support is dropped desugaring would be required.

cpovirk commented 3 years ago

Ouch, sorry. Thanks for letting us know. I think I was under the impression that desugaring of language features was on essentially universally nowadays, so this is information that I needed, though I certainly didn't want to discover it only by breaking stuff.

Do you know more about the state of adoption? For example:

(I will note in the release notes that we need to figure this out.)

ejona86 commented 3 years ago

In case it wasn't obvious, this was in the gRPC project.

I'm not too concerned about requiring desugaring of language features for Android. Although I sort of expected to start doing that when we drop Java 7 support; it is a bit harder to explain before that point.

I sincerely hope that gRPC's down-stream users are already desugaring; it is pretty painless and is quite helpful. But this would also be a new requirement, and I think you're all to aware of how those can be surprising :smile:. If Java 7 is really close to dead, I'd expect there to be more Android users without desugaring than Java 7 users. So this change might impact more users in a small way than the Java 7 drop later which will impact few users in a large way.

I'm very willing to follow your lead here. But I also thought you probably weren't aware of the issue. I think we'd message this as a pre-migration, making sure that Android users will suffer no ill effects from the Java 7 drop.

cpovirk commented 3 years ago

Thanks, that's all reassuring. Your "more users in a small way" / "few users in a large way" framing sounds like the right way of looking at it.

A pre-migration is indeed what we're going for. But I had hoped we could ease into it with some runtime warnings and then escalate from there, not immediately force compile-time errors on people.... Then again, I did specifically call out the possibility of compile-time errors with this change, so I guess I got my wish :) It would just be nice if we had a way to turn off this one error until later in the process. Maybe I should have started with Martin Buchholz‎'s suggestion to check java.class.version at runtime.

It's not yet clear to me whether we're a lead worth following or cautionary example. No one else has reported problems, but I can't imagine many people have adopted the new version yet. (Someday, adoption data may be available through deps.dev, but it's hard to conclude much from its existing data without seeing historical trends: From its snapshot data, I can see that the majority of Maven projects are on Guava 20.0 or earlier, which presumably means it's pulling in a lot of dead projects. We could probably get better data if we were sufficiently motivated.)

I will probably sit on this for a little: If someone turns up tomorrow and tells us that it's a big problem, it will be easy to decide to release 30.1.1 without the problematic class. If everything is still fine on Monday, then I should think harder about this.

ejona86 commented 3 years ago

It's not yet clear to me whether we're a lead worth following or cautionary example.

:rofl:

I will probably sit on this for a little:

Sounds fair. gRPC will be use 30.0 for the moment, which was still an upgrade for us. But our release isn't even scheduled for another month, and even then it seems fine to use 30.0. But we'll still want to make sure we figure out things soon-ish so that we can get users into a solid state to avoid being in a lurch if there is a security vulnerability, Guava's Important New Feature, or the like.

cpovirk commented 3 years ago

Unless we hear more reports of problems, I'm going to call this Working as Intended-ish.

I've updated our announcement and release notes to mention the change.

Some non-eventful updates:

I've been doing daily web searches from problems from this, and so far, they've only led me back here.

Plus, today, I did some searches of GitHub issues:

I went through 10 pages each from the first and third searches. I found one failure related to lack of desugaring. I left a comment there for the maintainer.

If anyone reading this does have a problem with the new requirement of desugaring, please let us know here.

cpovirk commented 3 years ago

Closing to indicate that we're not currently planning to change anything in response to the gRPC report specifically. But please continue to report any discoveries here so that we can change course as needed.