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

Upgrade to Guava 30.1, which warns on Java 7 #8100

Merged
merged 1 commit into from Apr 19, 2021
Merged

Conversation

ejona86
Copy link
Member

@ejona86 ejona86 commented Apr 16, 2021

This change can have large impact from two aspects:

  1. It calls out a large impact on the few Java 7 users.
  2. It may have small impact on the many Android users.

#4671 tracks gRPC's removal of
Java 7 support. We are quite eager to drop Java 7 support as that would
allow using new language features like default methods. Guava is also
dropping Java 7 support and starting in 30.1 it will warn when used on
Java 7. The purpose of the warning is to help discover users that are
negatively impacted by dropping Java 7 before it becomes a bigger
problem.

The Guava logging check was implemented in such a way that there is an
optional class that uses Java 8 bytecode. While the class is optional at
runtime, the Android build system notices when dexing and fails if
Java 8 language featutres are not enabled. We believe this will not be a
problem for most Android users, but they may need to add to their build:

android {
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_1_8
        targetCompatibility JavaVersion.VERSION_1_8
    }
}

See also https://github.com/google/guava/releases/tag/v30.1


This came up in #8078. CC @suztomo

This change can have large impact from two aspects:
1. It calls out a _large_ impact on the _few_ Java 7 users.
2. It may have _small_ impact on the _many_ Android users.

grpc#4671 tracks gRPC's removal of
Java 7 support. We are quite eager to drop Java 7 support as that would
allow using new language features like default methods. Guava is also
dropping Java 7 support and starting in 30.1 it will warn when used on
Java 7. The purpose of the warning is to help discover users that are
negatively impacted by dropping Java 7 before it becomes a bigger
problem.

The Guava logging check was implemented in such a way that there is an
optional class that uses Java 8 bytecode. While the class is optional at
runtime, the Android build system notices when dexing and fails if
Java 8 language featutres are not enabled. We believe this will not be a
problem for most Android users, but they may need to add to their build:

```
android {
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_1_8
        targetCompatibility JavaVersion.VERSION_1_8
    }
}
```

See also https://github.com/google/guava/releases/tag/v30.1
@ejona86 ejona86 merged commit a81bf14 into grpc:master Apr 19, 2021
@ejona86 ejona86 deleted the guava-30.1 branch April 19, 2021 16:16
@elharo
Copy link
Contributor

elharo commented Apr 21, 2021

Does repositories.bzl need to be updated too? It's still on 29.0.

@ejona86
Copy link
Member Author

ejona86 commented Apr 21, 2021

Yes it does. I'll send out something.

@elharo
Copy link
Contributor

elharo commented Apr 21, 2021

Is there anything we can or should do to automate this? I know that updating dependencies in gRPC is relatively painful compared to other projects. Thoughts:

  1. Renovatebot (can it handle updating repositories.bzl too?)
  2. some sort of automated test for the bazel build in the CI
  3. a custom script to update build.gradle and repositories.bzl so we don't have to run a separate program to calculate SHA256 hashes
  4. Something else?

@ejona86
Copy link
Member Author

ejona86 commented Apr 21, 2021

Most of the pain of upgrading dependencies is due to resolutionStrategy.failOnVersionConflict(), which we do exclusively because of Maven's poor resolution ordering. It would be nice to have a checker that has fewer false positives, but I've not seen anything that would really be better.

If you don't have to fight failOnVersionConflict(), and aren't updating protobuf, then the process is a two line change: change the root build.gradle and repositories.bzl. Getting rid of all the jvm_maven_import_external()s in 0e0bcdf in favor of maven_install has made the Bazel side of the process trivial.

My very-slowly-making-progress goal is to get Gradle into a form where we are no longer using the libraries map. I actually poke at things every-other-quarter or so. Gradle has come a long way here since we started, and platforms may offer what we need. But we'd need to do testing and verify pom.xml output is correct. There's a few different gradle options here that help.

Once Gradle is in a nice-and-pretty regular shape (e.g., look at plugin management in settings.gradle), then we could have a bash script (sed+cmp) that verifies versions match.

I'd like to make upgrade dependencies part of the post-release process. But I think we'll need to do that manually. I've hoped multiple times that Gradle would have something built-in that helps, but last I looked at version pinning in Gradle it didn't quite match what we needed.

@ejona86
Copy link
Member Author

ejona86 commented Apr 21, 2021

a custom script to update build.gradle and repositories.bzl so we don't have to run a separate program to calculate SHA256 hashes

All of that is gone. It feels so good. See 0e0bcdf. Bazel is easy now; Gradle is the problem (in part because of Maven).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants