kennknowles commented on a change in pull request #13211: URL: https://github.com/apache/beam/pull/13211#discussion_r520871132
########## File path: sdks/java/container/common.gradle ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * License); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an AS IS BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +applyDockerNature() Review comment: It would be handy to have some reverse pointers from this file to where it is used, in case things move around. Since this file is essentially meant to be used as a parameterized entrypoint for the other two modules. I know it may seem obvious but someone without context coming here will take a minute to sort it out. ########## File path: sdks/java/container/build.gradle ########## @@ -86,51 +76,21 @@ licenseReport { renderers = [new JsonReportRenderer()] } -def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8' -docker { - name containerImageName( - name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk", - root: project.rootProject.hasProperty(["docker-repository-root"]) ? - project.rootProject["docker-repository-root"] : - project.docker_image_default_repo_root, - tag: project.rootProject.hasProperty(["docker-tag"]) ? - project.rootProject["docker-tag"] : project.sdk_version) - dockerfile project.file("./Dockerfile") - files "./build/" - buildArgs([ - 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) || - project.rootProject.hasProperty(["isRelease"]), - 'java_version': imageJavaVersion, - ]) +task buildAll { Review comment: Unused? ########## File path: sdks/java/container/common.gradle ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * License); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an AS IS BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +applyDockerNature() + +def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8' + +description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container" Review comment: I would also suggest potentially passing in the whole atomic name. Also maybe no need for "Python" in the name. ########## File path: sdks/java/container/java11/build.gradle ########## @@ -0,0 +1,28 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * License); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an AS IS BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +plugins { + id 'base' + id 'org.apache.beam.module' +} + +project.ext { + imageJavaVersion = '11' Review comment: My dream, which may be impossible, is that this can be passed as a parameter somehow to the `apply from:` line so that the control and data flow are more clear. ########## File path: sdks/java/container/build.gradle ########## @@ -86,51 +76,21 @@ licenseReport { renderers = [new JsonReportRenderer()] } -def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8' -docker { - name containerImageName( - name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk", - root: project.rootProject.hasProperty(["docker-repository-root"]) ? - project.rootProject["docker-repository-root"] : - project.docker_image_default_repo_root, - tag: project.rootProject.hasProperty(["docker-tag"]) ? - project.rootProject["docker-tag"] : project.sdk_version) - dockerfile project.file("./Dockerfile") - files "./build/" - buildArgs([ - 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) || - project.rootProject.hasProperty(["isRelease"]), - 'java_version': imageJavaVersion, - ]) +task buildAll { + dependsOn ':sdks:java:container:java8:docker' + dependsOn ':sdks:java:container:java11:docker' } task pullLicenses(type: Exec) { + dependsOn generateLicenseReport + generateLicenseReport.outputs.cacheIf { false } Review comment: I noticed when I was fixing this file up that the dependency report was being pulled from the build cache. I guess it doesn't depend on the `build.gradle` so when deps change or when the config changes it doesn't update the report. I could be wrong. It may have been a temporary problem related to the particular change I was making. ########## File path: sdks/java/container/common.gradle ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * License); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an AS IS BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +applyDockerNature() + +def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8' + +description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container" + +configurations { + dockerDependency + sdkHarnessLauncher + pulledLicenses +} + +dependencies { + pulledLicenses project(path: ":sdks:java:container", configuration: "pulledLicenses") + dockerDependency project(path: ":sdks:java:container", configuration: "dockerDependency") + sdkHarnessLauncher project(path: ":sdks:java:container", configuration: "sdkHarnessLauncher") +} + +task copyDockerfileDependencies(type: Copy) { + from configurations.dockerDependency + rename "slf4j-api.*", "slf4j-api.jar" + rename "slf4j-jdk14.*", "slf4j-jdk14.jar" + rename 'beam-sdks-java-harness-.*.jar', 'beam-sdks-java-harness.jar' + rename 'beam-sdks-java-io-kafka.*.jar', 'beam-sdks-java-io-kafka.jar' + rename 'kafka-clients.*.jar', 'kafka-clients.jar' + into "build/target" +} + +task copySdkHarnessLauncher(type: Copy) { + from configurations.sdkHarnessLauncher + into "build/target" +} + +task copyPulledLicenses(type: Copy) { + from configurations.pulledLicenses + into "build/target" +} + +task copyGolangLicenses(type: Copy) { + from "${project(':release:go-licenses:java').buildDir}/output" + into "build/target/go-licenses" + dependsOn ':release:go-licenses:java:createLicenses' +} + +task skipPullLicenses(type: Exec) { + executable "sh" + args "-c", "mkdir -p build/target/third_party_licenses && touch build/target/third_party_licenses/skip" +} + +docker { + name containerImageName( + name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk", Review comment: Same comment about trying to keep names a bit less constructed by string append. I can see that the Java version is used in a bunch of places that need to match, so I can accept that one being injected. How much of this could go in e.g. `applyDockerNature(...)` ? I don't like how all of our different types of builds are defined on one monolithic file (BeamModulePlugin) with a bunch of cross-dependencies. At some point I'd love them to be factored apart into more independent plugins. But relative to the status quo, this file does seem like another kind of "nature". So this adds another kind of composition for folks to know about. In fairness, this composition is simpler and cleaner than the monolithic file. However, this is exactly how the monolithic file came to be: we had an `apply from:...` situation that grew and grew until it was impossible to deal with. ########## File path: sdks/java/container/common.gradle ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * License); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an AS IS BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +applyDockerNature() + +def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8' Review comment: I suggest making this non-optional and failing if the file is included by a project that did not pass the parameter. ########## File path: runners/portability/java/build.gradle ########## @@ -214,7 +214,7 @@ def createUlrValidatesRunnerTask = { name, environmentType -> } if (environmentType == "DOCKER") { - vrTask.dependsOn ":sdks:java:container:docker" + vrTask.dependsOn ":sdks:java:container:java8:docker" Review comment: Seems just as easy to make it a parameter. You could make the `$ver` a parameter as elsewhere in this PR or you could go further and pass in the name of the target to depend on. Pro: if anyone renames the target they can search and find the fact that it is depended on. Con: if anyone renames the target they do have to change it in all those places so if done manually it is more work. Also the control flow reading is a little more complex. All in all, I dislike mechanical assembly of target names, whether for definition or execution. So I prefer treating the whole target name as an atomic value and passing it in. ---------------------------------------------------------------- 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]
