-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 error_prone_annotations to 2.3.4 #6574
Conversation
server_urls = ["https://repo.maven.apache.org/maven2/"], | ||
artifact_sha256 = "ec59f1b702d9afc09e8c3929f5c42777dec623a6ea2731ac694332c7d7680f5a", | ||
artifact_sha256 = "baf7d6ea97ce606c53e11b6854ba5f2ce7ef5c24dddf0afa18d1260bd25b002c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suztomo@suxtomo24:~/grpc-java$ shasum -a 256 ~/Downloads/error_prone_annotations-2.3.4.jar
baf7d6ea97ce606c53e11b6854ba5f2ce7ef5c24dddf0afa18d1260bd25b002c /usr/local/google/home/suztomo/Downloads/error_prone_annotations-2.3.4.jar
Investigating the error.
|
there are few more places to update versions
also, looks like |
@creamsoup Thank you for the guide. I'll wait for #6566 then.
|
libraries.errorprone // to avoid using perfmark's dependency | ||
compile (libraries.perfmark) { | ||
exclude group: 'com.google.errorprone', module: 'error_prone_annotations' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gradle complained perfmark's errorprone annotation 2.3.3.
https://travis-ci.org/grpc/grpc-java/jobs/630091143?utm_medium=notification&utm_source=github_status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. You did the appropriate thing here, although we normally put a comment saying we are using version X instead of version Y (so that in the future we can tell if the exclude is old and should be removed).
Our build enforces dependency convergence because I've not found a better approach to detecting requireUpperBoundDeps violations from within Gradle (because they aren't a problem in Gradle...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment as "prefer our version to perfmark's 2.3.3".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be done in isolation. It requires an upgrade to guava and protobuf at the same time, and that means we need a protobuf release that updates its guava and errorprone first.
@elharo I see you already raised a PR for protobuf-java on Guava upgrade. Thanks. |
@elharo, why do you say this "requires an upgrade to guava and protobuf at the same time"? |
Still build failure due to GuardedBy. I'll wait for #6578.
|
It looks like my #6553 will get in before this one. If so, then this will need to be updated to change |
Because Guava and protobuf also pull in errorprone_annotations, and we'd like them to be in sync as much as possible. I don't think there's anything we actually need in error_prone_annotations 2.3.4. The reason to do this update in the first place is to bring everything into sync. |
Upgrading everything in lockstep is silly and not really productive. Bumping versions when new ones come out is fine, but saying we need to bump guava and protobuf at the same time just adds risk and doesn't help anyone. If we happen to bump all of the at the same time, that's fine. But grpc-java should not be party to any goals of dependency convergence. |
Done. Travis complains: https://travis-ci.org/grpc/grpc-java/jobs/631065748?utm_medium=notification&utm_source=github_status
|
@@ -122,6 +122,8 @@ | |||
// $ sudo tail -f /var/log/squid/access.log | |||
|
|||
private static final Logger log = Logger.getLogger(ProxyDetectorImpl.class.getName()); | |||
|
|||
@SuppressWarnings("UnnecessaryAnonymousClass") // grpc-java targets Java 7 (no method references) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis CI complained: https://travis-ci.org/grpc/grpc-java/jobs/631065748?utm_medium=notification&utm_source=github_status
> Task :grpc-core:compileJava
/home/travis/build/grpc/grpc-java/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java:125: warning: [UnnecessaryAnonymousClass] Implementing a functional interface is unnecessary; prefer to implement the functional interface method directly and use a method reference instead.
private static final AuthenticationProvider DEFAULT_AUTHENTICATOR = new AuthenticationProvider() {
^
(see https://errorprone.info/bugpattern/UnnecessaryAnonymousClass)
Did you mean 'private static PasswordAuthentication defaultAuthenticator('?
/home/travis/build/grpc/grpc-java/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java:143: warning: [UnnecessaryAnonymousClass] Implementing a functional interface is unnecessary; prefer to implement the functional interface method directly and use a method reference instead.
private static final Supplier<ProxySelector> DEFAULT_PROXY_SELECTOR =
^
(see https://errorprone.info/bugpattern/UnnecessaryAnonymousClass)
Did you mean 'private static ProxySelector defaultProxySelector() {'?
error: warnings found and -Werror specified
1 error
2 warnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppressing this looks appropriate. There's a reasonable number of these. In the future if there are lots of a particular failure that doesn't make sense like this you can disable the check altogether. See what we've done for JdkObsolete
in the main build.gradle for an example of that. In this case it's on the border of silencing them globally or in each file (so what you've done is fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the guidance. I choose to suppress UnnecessaryAnonymousClass
at build.gradle because the decision of the suppression is project level (choosing Java 7 compatible source; no method references).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis CI passes. Would you merge this?
@@ -122,6 +122,8 @@ | |||
// $ sudo tail -f /var/log/squid/access.log | |||
|
|||
private static final Logger log = Logger.getLogger(ProxyDetectorImpl.class.getName()); | |||
|
|||
@SuppressWarnings("UnnecessaryAnonymousClass") // grpc-java targets Java 7 (no method references) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the guidance. I choose to suppress UnnecessaryAnonymousClass
at build.gradle because the decision of the suppression is project level (choosing Java 7 compatible source; no method references).
@@ -586,10 +587,12 @@ void frameHeader(int streamId, int length, byte type, byte flags) throws IOExcep | |||
} | |||
} | |||
|
|||
@FormatMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build failed by https://errorprone.info/bugpattern/AnnotateFormatMethod
@@ -169,6 +169,7 @@ public Deadline offset(long offset, TimeUnit units) { | |||
* {@link TimeUnit#convert}. If there is no time remaining, the returned duration is how | |||
* long ago the deadline expired. | |||
*/ | |||
@SuppressWarnings("PreferJavaTimeOverload") // Duration class not available in Java 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build for Java 11 failed without this suppresswarnings.
> Task :grpc-context:compileJava
/home/travis/build/grpc/grpc-java/context/src/main/java/io/grpc/Deadline.java:177: warning: [PreferJavaTimeOverload] Prefer using java.time-based APIs when available. Note that this checker does not and cannot guarantee that the overloads have equivalent semantics, but that is generally the case with overloaded methods.
return unit.convert(deadlineNanos - nowNanos, TimeUnit.NANOSECONDS);
^
(see https://errorprone.info/bugpattern/PreferJavaTimeOverload)
Did you mean 'return unit.convert(Duration.ofNanos(deadlineNanos - nowNanos));'?
error: warnings found and -Werror specified
1 error
1 warning
https://travis-ci.org/grpc/grpc-java/jobs/631194184?utm_medium=notification&utm_source=github_status
plugins.withId("java") { | ||
compileJmhJava { | ||
// This project targets Java 7 (no method references) | ||
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc-core has special task compileJmhJava
. Without this configuration, the build failed:
> Task :grpc-core:compileJmhJava FAILED
/Users/suztomo/grpc-java/core/src/jmh/java/io/grpc/internal/SerializingExecutorBenchmark.java:58: warning: [UnnecessaryAnonymousClass] Implementing a functional interface is unnecessary; prefer to implement the functional interface method directly and use a method reference instead.
private final Runnable phaserRunnable = new Runnable() {
^
(see https://errorprone.info/bugpattern/UnnecessaryAnonymousClass)
Did you mean 'private void phaserRunnable() {'?
error: warnings found and -Werror specified
1 error
1 warning
@@ -603,7 +603,7 @@ public void callsAndShutdownAndShutdownNow() { | |||
} | |||
|
|||
private void subtestCallsAndShutdown(boolean shutdownNow, boolean shutdownNowAfterShutdown) { | |||
FakeNameResolverFactory nameResolverFactory = | |||
ManagedChannelImplTest.FakeNameResolverFactory nameResolverFactory = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SameNameButDifferent.
// This project targets Java 7 (no method references) | ||
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF) | ||
// This project targets Java 7 (no time.Duration class) | ||
options.errorprone.check("PreferJavaTimeOverload", CheckSeverity.OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without PreferJavaTimeOverload suppression, the build for Java 11 fails:
> Task :grpc-context:compileJava FAILED
/Users/suztomo/grpc-java/context/src/main/java/io/grpc/Deadline.java:177: warning: [PreferJavaTimeOverload] Prefer using java.time-based APIs when available. Note that this checker does not and cannot guarantee that the overloads have equivalent semantics, but that is generally the case with overloaded methods.
return unit.convert(deadlineNanos - nowNanos, TimeUnit.NANOSECONDS);
^
(see https://errorprone.info/bugpattern/PreferJavaTimeOverload)
Did you mean 'return unit.convert(Duration.ofNanos(deadlineNanos - nowNanos));'?
error: warnings found and -Werror specified
1 error
1 warning
Setting this suppression at project level (rather than annotation) because whole grpc-java project targets Java 7, which does not have java.time.Duration.
@@ -127,6 +127,7 @@ public void closed( | |||
} | |||
|
|||
@Test | |||
@SuppressWarnings("GuardedBy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this suppression, the build fails.
https://travis-ci.org/grpc/grpc-java/jobs/631198698?utm_medium=notification&utm_source=github_status
#6578 will take care of the root cause.
@@ -127,7 +126,7 @@ public void wrapChannel_handler() throws Exception { | |||
MethodDescriptor<RequestT, ResponseT> methodDescriptor, CallOptions callOptions) { | |||
return new NoopClientCall<RequestT, ResponseT>() { | |||
@Override | |||
public void start(Listener<ResponseT> responseListener, Metadata headers) { | |||
public void start(ClientCall.Listener<ResponseT> responseListener, Metadata headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SameNameButDifferent
@@ -279,9 +279,17 @@ subprojects { | |||
} | |||
} | |||
|
|||
compileJava { | |||
// This project targets Java 7 (no method references) | |||
options.errorprone.check("UnnecessaryAnonymousClass", CheckSeverity.OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppressing UnnecessaryAnonymousClass as project-level suppression, because grpc-java targets Java 7 source.
libraries.errorprone // to avoid using perfmark's dependency | ||
compile (libraries.perfmark) { | ||
exclude group: 'com.google.errorprone', module: 'error_prone_annotations' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment as "prefer our version to perfmark's 2.3.3".
I see Android and Bazel build failed. Will look into these. |
@dapengzhang0 Would you run presubmit Kokoro check for the latest commit (ef42e8b)? 749b67f passed Bazel test. |
dapengzhang0 Thanks! Now all checks have passed (13 successful checks). Would you merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this has been the most painful upgrade of Error Prone in a while. Thank you for the work. I think we can reduce the diff size considerably though with two small renames.
@@ -606,7 +606,7 @@ public void callsAndShutdownAndShutdownNow() { | |||
} | |||
|
|||
private void subtestCallsAndShutdown(boolean shutdownNow, boolean shutdownNowAfterShutdown) { | |||
FakeNameResolverFactory nameResolverFactory = | |||
ManagedChannelImplTest2.FakeNameResolverFactory nameResolverFactory = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing all of these names, could we instead just change the one other name. It looks like duplicate name is because of the class FakeNameResolverFactory
on line 3743. Let's just change that one class's name. Worst-case, name it FakeNameResolverFactory2
.
Ditto on line 3452 in ManagedChannelImplTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ejona86 PTAL.
@@ -3417,7 +3417,7 @@ public void start(Listener2 listener) { | |||
public void shutdown() {} | |||
} | |||
|
|||
final class FakeNameResolverFactory extends NameResolver.Factory { | |||
final class FakeNameResolverFactory2 extends NameResolver.Factory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to FakeNameResolverFactory2 to avoid SameNameButDifferent.
@@ -606,7 +606,7 @@ public void callsAndShutdownAndShutdownNow() { | |||
} | |||
|
|||
private void subtestCallsAndShutdown(boolean shutdownNow, boolean shutdownNowAfterShutdown) { | |||
FakeNameResolverFactory nameResolverFactory = | |||
ManagedChannelImplTest2.FakeNameResolverFactory nameResolverFactory = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice idea. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much smaller diff size!
@suztomo, thank you! |
Thank you for the review. |
Upgrading error_prone_annotations dependency from 2.3.3 to 2.3.4.
Motivation
Guava 28.2 has error_prone_annotations 2.3.4. An upper-bound-check detected gRPC has 2.3.3:
googleapis/java-websecurityscanner#32 (comment)