kennknowles commented on a change in pull request #12938:
URL: https://github.com/apache/beam/pull/12938#discussion_r537730018
##########
File path: runners/core-construction-java/build.gradle
##########
@@ -60,8 +60,9 @@ dependencies {
compile library.java.jackson_databind
compile library.java.joda_time
compile library.java.slf4j_api
- testCompile library.java.hamcrest_core
- testCompile library.java.hamcrest_library
+ compile "com.fasterxml.jackson.core:jackson-annotations:2.10.2"
Review comment:
Should be `library.jackson_core` etc
##########
File path: runners/core-java/build.gradle
##########
@@ -41,17 +41,17 @@ test {
dependencies {
compile project(path: ":model:pipeline", configuration: "shadow")
compile project(path: ":sdks:java:core", configuration: "shadow")
- compile project(path: ":model:fn-execution", configuration: "shadow")
+ compile project(path: ":model:job-management:", configuration: "shadow")
compile project(":runners:core-construction-java")
- compile project(":sdks:java:fn-execution")
+ permitUsedUndeclared project(":sdks:java:fn-execution")
Review comment:
We should not need `permitUsedDeclared` for this. It seems like it was
OK before. Otherwise at least it would be `runtime`.
##########
File path: runners/flink/1.10/job-server/build.gradle
##########
@@ -29,3 +29,7 @@ project.ext {
// Load the main build script which contains all build logic.
apply from: "$basePath/flink_job_server.gradle"
+
+dependencies {
+ permitUnusedDeclared project(":runners:flink:1.10")
Review comment:
This seems strange. The Flink 1.10 job server should definitely actually
use `:runners:flink:1.10`. And same for the other versions. Why is this needed?
##########
File path: runners/direct-java/build.gradle
##########
@@ -70,17 +71,16 @@ dependencies {
shadow library.java.vendored_grpc_1_26_0
shadow library.java.joda_time
shadow library.java.slf4j_api
- shadow library.java.args4j
+ permitUnusedDeclared "org.apache.beam:beam-vendor-grpc-1_26_0:0.3"
+ permitUnusedDeclared project(":runners:java-fn-execution")
+ permitUnusedDeclared project(":sdks:java:fn-execution")
Review comment:
Transitive dependencies are automatically included. So there is
something else that makes these transitive dependencies not work. If the
compile step succeeds but the test step does not succeed, that means it is a
runtime dependency of the test. I think the only thing we can do is to add the
exclusion, for now. Please add a comment that these are runtime dependencies of
the tests.
##########
File path:
buildSrc/src/main/groovy/org/apache/beam/gradle/GrpcVendoring_1_26_0.groovy
##########
@@ -43,40 +43,13 @@ class GrpcVendoring_1_26_0 {
static def npn_api_version = "1.1.1.v20141010"
static def jboss_marshalling_version = "1.4.11.Final"
static def jboss_modules_version = "1.1.0.Beta1"
-
/** Returns the list of compile time dependencies. */
static List<String> dependencies() {
return [
- "com.google.guava:guava:$guava_version",
Review comment:
It is not quite so simple. See
https://cwiki.apache.org/confluence/display/BEAM/Dependency+Upgrades
Specifically,
./gradlew -Ppublishing -PjavaLinkageArtifactIds=beam-vendor-grpc-1_26_0
:checkJavaLinkage
##########
File path:
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
##########
@@ -850,7 +849,7 @@ class BeamModulePlugin implements Plugin<Project> {
// configurations because they are never required to be shaded or become
a
// dependency of the output.
def compileOnlyAnnotationDeps = [
- "com.google.auto.value:auto-value-annotations:1.7",
+ "com.google.auto.value:auto-value-annotations:1.7.2",
Review comment:
You gave a thumbs up but did not add the lines here :-)
Here is what I mean: Right now you have this line in many `build.gradle`
files:
permitUnusedDeclared "com.google.auto.value:auto-value-annotations:1.7.2"
Instead, move that line here. This way the `build.gradle` files are cleaner
and do not have to have exclusions for things they do not control.
##########
File path: runners/google-cloud-dataflow-java/worker/windmill/build.gradle
##########
@@ -36,3 +36,9 @@ configurations {
artifacts {
unshaded jar
}
+
+dependencies {
+ compile "com.google.guava:guava:28.1-android"
Review comment:
This should be `library.guava` (here and everywhere)
If that version doesn't work, we need to know why.
##########
File path: sdks/java/io/file-based-io-tests/build.gradle
##########
@@ -31,8 +31,7 @@ dependencies {
testCompile project(path: ":sdks:java:io:parquet", configuration:
"testRuntime")
testCompile project(path: ":sdks:java:testing:test-utils", configuration:
"testRuntime")
testCompile library.java.jaxb_api
- testCompile library.java.jaxb_impl
testCompile library.java.junit
- testCompile library.java.hamcrest_core
testCompile library.java.hadoop_client
+ permitUnusedDeclared "org.checkerframework:checker-qual:3.7.0"
Review comment:
Choosing one file at random to comment on: do this in BeamModulePlugin
not in every build.gradle file.
##########
File path: examples/java/build.gradle
##########
@@ -67,7 +67,11 @@ dependencies {
compile library.java.joda_time
compile library.java.proto_google_cloud_datastore_v1
compile library.java.slf4j_api
- compile library.java.slf4j_jdk14
+ compile "com.google.api.grpc:proto-google-cloud-language-v1:1.81.4"
+ compile "com.google.code.findbugs:jsr305:3.0.2"
+ compile "com.google.oauth-client:google-oauth-client:1.31.0"
+ permitUnusedDeclared "com.google.auto.value:auto-value-annotations:1.7.2"
+ permitUnusedDeclared "org.checkerframework:checker-qual:3.7.0"
Review comment:
... but not this one?
I wonder if you forgot to push the new commit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
