-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
all: migrate gradle build to java-library plugin #6902
Conversation
2ae4da0
to
3dcd610
Compare
3dcd610
to
d94a910
Compare
@ejona86 Now is ready for review. PTAL. |
80b89ac
to
9630645
Compare
9630645
to
6b4b151
Compare
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 got to interop-testing before sort of giving up. The unnecessary changes make this at least 10x harder to review. It is probably best to reduce the diff to only the necessary changes before resuming review.
What I got through I feel fairly confident about. So I could believe one review is enough. But there is just so much noise. It seems like this would have been a fairly easy PR to review without the sorting and rearrangements.
} | ||
compile libraries.netty_tcnative |
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.
This looks like it was removed. It seems it should be a runtimeOnly dep.
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.
Does gae-interop-testing really need 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.
Yeah, we use tcnative on GAE Java 8 and want to make sure it works, since most people will be using grpc-netty-shaded which includes it. Since v1.25.0 the test probably passes without tcnative, though, since GAE Java 8 includes Conscrypt. But we still want to test with tcnative.
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 see. Add it add as runtimeOnly dep in #7001 now.
@@ -11,17 +11,17 @@ description = "gRPC: GRPCLB LoadBalancer plugin" | |||
evaluationDependsOn(project(':grpc-core').path) | |||
|
|||
dependencies { | |||
compile project(':grpc-core'), | |||
compileOnly libraries.javax_annotation | |||
implementation project(':grpc-core'), | |||
project(':grpc-protobuf'), | |||
project(':grpc-stub'), |
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'm surprised this trailing comma doesn't cause parsing problems. (Travis isn't failing, so it seems it really isn't a problem, but it makes me wonder.)
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 line below was accidentally removed by mistake. I think it doesn't cause parsing problems because the next line's return value is appended to the current list.
// prefer our own versions instead of protobuf-util's dependency | ||
exclude group: 'com.google.guava', module: 'guava' | ||
implementation (libraries.protobuf_util) { | ||
// prefer our own version from grpc-stub instead of protobuf-util's dependency | ||
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.
We need to add explicitly dependencies on errorprone and guava to match these excluded ones.
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.
Done in #7001
project(':grpc-protobuf'), | ||
project(':grpc-stub'), | ||
libraries.protobuf |
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.
Why removed direct dependency of protobuf? (Although seems not to break). But since it is an implementation dependency, it might be fine to just rely on the transitive.
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 think it was accidentally removed by mistake. Corrected it in #7001.
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.
Rebased and fixed the comments in #7001.
project(':grpc-protobuf'), | ||
project(':grpc-stub'), | ||
libraries.protobuf |
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 think it was accidentally removed by mistake. Corrected it in #7001.
@@ -11,17 +11,17 @@ description = "gRPC: GRPCLB LoadBalancer plugin" | |||
evaluationDependsOn(project(':grpc-core').path) | |||
|
|||
dependencies { | |||
compile project(':grpc-core'), | |||
compileOnly libraries.javax_annotation | |||
implementation project(':grpc-core'), | |||
project(':grpc-protobuf'), | |||
project(':grpc-stub'), |
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 line below was accidentally removed by mistake. I think it doesn't cause parsing problems because the next line's return value is appended to the current list.
// prefer our own versions instead of protobuf-util's dependency | ||
exclude group: 'com.google.guava', module: 'guava' | ||
implementation (libraries.protobuf_util) { | ||
// prefer our own version from grpc-stub instead of protobuf-util's dependency | ||
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.
Done in #7001
} | ||
compile libraries.netty_tcnative |
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.
Does gae-interop-testing really need this?
Closing in favor of #7001. |
api
for dependencies that are part of grpc public api signatures.compile
,testCompile
,runtime
andtestRuntime
.error_prone_annotations
is an exception: It is also excluded but manually added not as runtimeOnly. It must always compile with guava, otherwise users will see warning spams if guava is in the compile classpath but error_prone_annotations is not (See Many warning messages related to errorprone "CompatibleWith" google/guava#2721).