From 2645e3a9c09660d2a6afd6e61fe23c8042afabbc Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Sat, 2 Sep 2023 19:12:13 -0400 Subject: [PATCH 1/6] Partially revert #28234 to fix Java 11 Example PreCommit --- .../groovy/org/apache/beam/gradle/BeamModulePlugin.groovy | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index ba090bc556c0..6f9bca60a50c 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -1089,8 +1089,9 @@ class BeamModulePlugin implements Plugin { options.fork = true options.forkOptions.javaHome = java11Home as File options.compilerArgs += ['-Xlint:-path'] - options.compilerArgs += ['-source', '11'] - options.compilerArgs += ['-target', '11'] + // TODO(https://github.com/apache/beam/pull/28234) Investigate why different options need to be set in Java 11/17 + // Java11: --release 11; Java17: -source 17 -target 17; (Java 8 both work) + options.compilerArgs.addAll(['--release', '11']) } project.tasks.withType(Test) { useJUnit() From ca124360aa2fc2cb96172346652b27a9c1a2c95c Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Tue, 5 Sep 2023 12:49:59 -0400 Subject: [PATCH 2/6] Dedup compile args --- .../beam/gradle/BeamModulePlugin.groovy | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index 6f9bca60a50c..ceebf062eb33 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -451,6 +451,28 @@ class BeamModulePlugin implements Plugin { return 'beam' + p.path.replace(':', '-') } + /* + * Set compile args for compiling and running in different java version by modifying the compiler args in place. + * + * If there is already `-source X` and `-target X` option, replace X with desired version number. + * Otherwise, append them. + */ + def setCompileAndRuntimeJavaVersion(List compileArgs, String ver) { + boolean foundS = false, foundT = false + for (int i = 0; i < compileArgs.size()-1; ++i) { + if (compileArgs.get(i) == '-source' || compileArgs.get(i) == '-target') { + found = true + compileArgs.set(i+1, ver) + } + } + if (!foundS) { + compilerArgs += ['-source', ver] + } + if (!foundT) { + compilerArgs += ['-target', ver] + } + } + void apply(Project project) { /** ***********************************************************************************************/ @@ -1060,8 +1082,7 @@ class BeamModulePlugin implements Plugin { // artifacts. See https://stackoverflow.com/a/43103038/4368200 for additional details. if (JavaVersion.VERSION_1_8.compareTo(JavaVersion.toVersion(project.javaVersion)) == 0 && JavaVersion.VERSION_1_8.compareTo(JavaVersion.current()) < 0) { - options.compilerArgs += ['-source', '8'] - options.compilerArgs += ['-target', '8'] + setCompileAndRuntimeJavaVersion(options.compilerArgs, '8') // TODO(https://github.com/apache/beam/issues/23901): Fix // optimizerOuterThis breakage options.compilerArgs += ['-XDoptimizeOuterThis=false'] @@ -1089,9 +1110,7 @@ class BeamModulePlugin implements Plugin { options.fork = true options.forkOptions.javaHome = java11Home as File options.compilerArgs += ['-Xlint:-path'] - // TODO(https://github.com/apache/beam/pull/28234) Investigate why different options need to be set in Java 11/17 - // Java11: --release 11; Java17: -source 17 -target 17; (Java 8 both work) - options.compilerArgs.addAll(['--release', '11']) + setCompileAndRuntimeJavaVersion(options.compilerArgs, '11') } project.tasks.withType(Test) { useJUnit() @@ -1491,8 +1510,7 @@ class BeamModulePlugin implements Plugin { if (project.hasProperty("compileAndRunTestsWithJava17")) { def java17Home = project.findProperty("java17Home") project.tasks.compileTestJava { - options.compilerArgs += ['-target', '17'] - options.compilerArgs += ['-source', '17'] + setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') project.ext.setJava17Options(options) } project.tasks.withType(Test) { From 2aae0bc4877bd59c24360b8674eacfe5c3502382 Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Tue, 5 Sep 2023 13:23:28 -0400 Subject: [PATCH 3/6] fix typo --- .../groovy/org/apache/beam/gradle/BeamModulePlugin.groovy | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index ceebf062eb33..a1a669b919af 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -457,12 +457,12 @@ class BeamModulePlugin implements Plugin { * If there is already `-source X` and `-target X` option, replace X with desired version number. * Otherwise, append them. */ - def setCompileAndRuntimeJavaVersion(List compileArgs, String ver) { + def setCompileAndRuntimeJavaVersion(List compilerArgs, String ver) { boolean foundS = false, foundT = false - for (int i = 0; i < compileArgs.size()-1; ++i) { - if (compileArgs.get(i) == '-source' || compileArgs.get(i) == '-target') { + for (int i = 0; i < compilerArgs.size()-1; ++i) { + if (compilerArgs.get(i) == '-source' || compilerArgs.get(i) == '-target') { found = true - compileArgs.set(i+1, ver) + compilerArgs.set(i+1, ver) } } if (!foundS) { From 1ca7a884d23db233663a4725b5b35de9927c2a8d Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Tue, 5 Sep 2023 15:12:36 -0400 Subject: [PATCH 4/6] Fix compilerArgs addAll --- ...reCommit_Java_Examples_Dataflow_Java11.yml | 2 + .../beam/gradle/BeamModulePlugin.groovy | 49 ++++++++++--------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/.github/workflows/beam_PreCommit_Java_Examples_Dataflow_Java11.yml b/.github/workflows/beam_PreCommit_Java_Examples_Dataflow_Java11.yml index 39910471a8f4..8f8a3bba405c 100644 --- a/.github/workflows/beam_PreCommit_Java_Examples_Dataflow_Java11.yml +++ b/.github/workflows/beam_PreCommit_Java_Examples_Dataflow_Java11.yml @@ -100,6 +100,8 @@ jobs: service_account_key: ${{ secrets.GCP_SA_KEY }} project_id: ${{ secrets.GCP_PROJECT_ID }} export_default_credentials: true + # The workflow installs java 11 and as default jvm. This is different from + # PreCommit_Java_Examples_Dataflow_Java17 where the build system and sources are compiled with Java8 - name: Set up Java uses: actions/setup-java@v3.8.0 with: diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index a1a669b919af..643570f9d863 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -18,23 +18,24 @@ package org.apache.beam.gradle +import static java.util.UUID.randomUUID + import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar import groovy.json.JsonOutput import groovy.json.JsonSlurper +import java.net.ServerSocket +import java.util.logging.Logger import org.gradle.api.attributes.Category import org.gradle.api.GradleException import org.gradle.api.JavaVersion import org.gradle.api.Plugin import org.gradle.api.Project -import org.gradle.api.Task import org.gradle.api.artifacts.Configuration import org.gradle.api.artifacts.ProjectDependency import org.gradle.api.file.FileCollection import org.gradle.api.file.FileTree -import org.gradle.api.plugins.quality.Checkstyle import org.gradle.api.publish.maven.MavenPublication import org.gradle.api.tasks.Copy -import org.gradle.api.tasks.Delete import org.gradle.api.tasks.Exec import org.gradle.api.tasks.JavaExec import org.gradle.api.tasks.TaskProvider @@ -43,13 +44,10 @@ import org.gradle.api.tasks.compile.CompileOptions import org.gradle.api.tasks.compile.JavaCompile import org.gradle.api.tasks.javadoc.Javadoc import org.gradle.api.tasks.testing.Test -import org.gradle.api.tasks.PathSensitive import org.gradle.api.tasks.PathSensitivity import org.gradle.testing.jacoco.tasks.JacocoReport -import java.net.ServerSocket -import static java.util.UUID.randomUUID /** * This plugin adds methods to configure a module with Beam's defaults, called "natures". * @@ -67,6 +65,8 @@ import static java.util.UUID.randomUUID */ class BeamModulePlugin implements Plugin { + static final Logger logger = Logger.getLogger(BeamModulePlugin.class.getName()) + /** Licence header enforced by spotless */ static final String javaLicenseHeader = """/* * Licensed to the Apache Software Foundation (ASF) under one @@ -457,19 +457,23 @@ class BeamModulePlugin implements Plugin { * If there is already `-source X` and `-target X` option, replace X with desired version number. * Otherwise, append them. */ - def setCompileAndRuntimeJavaVersion(List compilerArgs, String ver) { + static def setCompileAndRuntimeJavaVersion(List compilerArgs, String ver) { boolean foundS = false, foundT = false + logger.fine("set java ver ${ver} to compiler args") for (int i = 0; i < compilerArgs.size()-1; ++i) { - if (compilerArgs.get(i) == '-source' || compilerArgs.get(i) == '-target') { - found = true + if (compilerArgs.get(i) == '-source') { + foundS = true + compilerArgs.set(i+1, ver) + } else if (compilerArgs.get(i) == '-target') { + foundT = true compilerArgs.set(i+1, ver) } } if (!foundS) { - compilerArgs += ['-source', ver] + compilerArgs.addAll('-source', ver) } if (!foundT) { - compilerArgs += ['-target', ver] + compilerArgs.addAll('-target', ver) } } @@ -1116,6 +1120,16 @@ class BeamModulePlugin implements Plugin { useJUnit() executable = "${java11Home}/bin/java" } + } else if (project.hasProperty("compileAndRunTestsWithJava17")) { + def java17Home = project.findProperty("java17Home") + project.tasks.compileTestJava { + setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') + project.ext.setJava17Options(options) + } + project.tasks.withType(Test) { + useJUnit() + executable = "${java17Home}/bin/java" + } } // Configure the default test tasks set of tests executed @@ -1254,8 +1268,6 @@ class BeamModulePlugin implements Plugin { } } - - if (configuration.shadowClosure) { // Ensure that tests are packaged and part of the artifact set. project.task('packageTests', type: Jar) { @@ -1507,17 +1519,6 @@ class BeamModulePlugin implements Plugin { options.errorprone.errorproneArgs.add("-Xep:Slf4jLoggerShouldBeNonStatic:OFF") } - if (project.hasProperty("compileAndRunTestsWithJava17")) { - def java17Home = project.findProperty("java17Home") - project.tasks.compileTestJava { - setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') - project.ext.setJava17Options(options) - } - project.tasks.withType(Test) { - useJUnit() - executable = "${java17Home}/bin/java" - } - } if (configuration.shadowClosure) { // Enables a plugin which can perform shading of classes. See the general comments // above about dependency management for Java projects and how the shadow plugin From de8e6800eda5ade27c4d7ad33cefcdeae97c7fef Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Tue, 5 Sep 2023 16:13:10 -0400 Subject: [PATCH 5/6] Move compileAndRunTestsWithJavaX after errorprone has set up --- .../beam/gradle/BeamModulePlugin.groovy | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index 643570f9d863..27100791a321 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -1108,30 +1108,6 @@ class BeamModulePlugin implements Plugin { preserveFileTimestamps(false) } - if (project.hasProperty("compileAndRunTestsWithJava11")) { - def java11Home = project.findProperty("java11Home") - project.tasks.compileTestJava { - options.fork = true - options.forkOptions.javaHome = java11Home as File - options.compilerArgs += ['-Xlint:-path'] - setCompileAndRuntimeJavaVersion(options.compilerArgs, '11') - } - project.tasks.withType(Test) { - useJUnit() - executable = "${java11Home}/bin/java" - } - } else if (project.hasProperty("compileAndRunTestsWithJava17")) { - def java17Home = project.findProperty("java17Home") - project.tasks.compileTestJava { - setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') - project.ext.setJava17Options(options) - } - project.tasks.withType(Test) { - useJUnit() - executable = "${java17Home}/bin/java" - } - } - // Configure the default test tasks set of tests executed // to match the equivalent set that is executed by the maven-surefire-plugin. // See http://maven.apache.org/components/surefire/maven-surefire-plugin/test-mojo.html @@ -1519,6 +1495,30 @@ class BeamModulePlugin implements Plugin { options.errorprone.errorproneArgs.add("-Xep:Slf4jLoggerShouldBeNonStatic:OFF") } + if (project.hasProperty("compileAndRunTestsWithJava11")) { + def java11Home = project.findProperty("java11Home") + project.tasks.compileTestJava { + options.fork = true + options.forkOptions.javaHome = java11Home as File + options.compilerArgs += ['-Xlint:-path'] + setCompileAndRuntimeJavaVersion(options.compilerArgs, '11') + } + project.tasks.withType(Test) { + useJUnit() + executable = "${java11Home}/bin/java" + } + } else if (project.hasProperty("compileAndRunTestsWithJava17")) { + def java17Home = project.findProperty("java17Home") + project.tasks.compileTestJava { + setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') + project.ext.setJava17Options(options) + } + project.tasks.withType(Test) { + useJUnit() + executable = "${java17Home}/bin/java" + } + } + if (configuration.shadowClosure) { // Enables a plugin which can perform shading of classes. See the general comments // above about dependency management for Java projects and how the shadow plugin From e28e95811081fd0f73304b70ebfeedcc6cfb0514 Mon Sep 17 00:00:00 2001 From: Yi Hu Date: Wed, 6 Sep 2023 00:27:22 -0400 Subject: [PATCH 6/6] Still use --release for java8 option --- .../apache/beam/gradle/BeamModulePlugin.groovy | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy index 27100791a321..7d65ffa755d9 100644 --- a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy +++ b/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy @@ -454,11 +454,11 @@ class BeamModulePlugin implements Plugin { /* * Set compile args for compiling and running in different java version by modifying the compiler args in place. * - * If there is already `-source X` and `-target X` option, replace X with desired version number. - * Otherwise, append them. + * Replace `-source X` and `-target X` or `--release X` options if already existed in compilerArgs. */ static def setCompileAndRuntimeJavaVersion(List compilerArgs, String ver) { boolean foundS = false, foundT = false + int foundR = -1 logger.fine("set java ver ${ver} to compiler args") for (int i = 0; i < compilerArgs.size()-1; ++i) { if (compilerArgs.get(i) == '-source') { @@ -467,8 +467,14 @@ class BeamModulePlugin implements Plugin { } else if (compilerArgs.get(i) == '-target') { foundT = true compilerArgs.set(i+1, ver) + } else if (compilerArgs.get(i) == '--release') { + foundR = i } } + if (foundR != -1) { + compilerArgs.removeAt(foundR + 1) + compilerArgs.removeAt(foundR) + } if (!foundS) { compilerArgs.addAll('-source', ver) } @@ -1081,12 +1087,12 @@ class BeamModulePlugin implements Plugin { options.encoding = "UTF-8" // Use -source 8 -target 8 when targeting Java 8 and running on JDK > 8 // - // Consider migrating compilation and testing to use JDK 9+ and setting '-source 8 -target 8' as + // Consider migrating compilation and testing to use JDK 9+ and setting '--release 8' as // the default allowing 'applyJavaNature' to override it for the few modules that need JDK 9+ // artifacts. See https://stackoverflow.com/a/43103038/4368200 for additional details. if (JavaVersion.VERSION_1_8.compareTo(JavaVersion.toVersion(project.javaVersion)) == 0 && JavaVersion.VERSION_1_8.compareTo(JavaVersion.current()) < 0) { - setCompileAndRuntimeJavaVersion(options.compilerArgs, '8') + options.compilerArgs += ['--release', '8'] // TODO(https://github.com/apache/beam/issues/23901): Fix // optimizerOuterThis breakage options.compilerArgs += ['-XDoptimizeOuterThis=false'] @@ -1503,7 +1509,7 @@ class BeamModulePlugin implements Plugin { options.compilerArgs += ['-Xlint:-path'] setCompileAndRuntimeJavaVersion(options.compilerArgs, '11') } - project.tasks.withType(Test) { + project.tasks.withType(Test).configureEach { useJUnit() executable = "${java11Home}/bin/java" } @@ -1513,7 +1519,7 @@ class BeamModulePlugin implements Plugin { setCompileAndRuntimeJavaVersion(options.compilerArgs, '17') project.ext.setJava17Options(options) } - project.tasks.withType(Test) { + project.tasks.withType(Test).configureEach { useJUnit() executable = "${java17Home}/bin/java" }