From 3dc59e400a9e0a57f34aa990ec410a1b2af760d3 Mon Sep 17 00:00:00 2001 From: iirina Date: Fri, 31 May 2019 06:44:21 -0700 Subject: [PATCH] Fix jacoco-toolchain dependency. Bazel 0.26 added the `jacocorunner` attribute on `java_toolchain`. Java targets now fail when using the new host java_toolchain because of a dependency cycle: ``` @bazel_tools//tools/jdk:JacocoCoverageRunner @bazel_tools//tools/jdk:current_java_toolchain @bazel_tools//tools/jdk:legacy_current_java_toolchain @bazel_tools//tools/jdk:remote_toolchain @remote_java_tools_linux//:toolchain .-> @remote_java_tools_linux//:jacoco_coverage_runner (host) | @bazel_tools//tools/jdk:current_java_toolchain (host) | @bazel_tools//tools/jdk:legacy_current_java_toolchain (host) | @bazel_tools//tools/jdk:remote_toolchain (host) | @remote_java_tools_linux//:toolchain (host) `-- @remote_java_tools_linux//:jacoco_coverage_runner (host) ``` The dependency is happening because `jacocorunner` in `java_toolchain` was declared as a `java_import` which depends on the host `java_toolchain`. The issue was not caught because we only run the java integration tests with `--java_toolchain` built at head. If we had run the tests also with `--host_java_toolchain` the issue would have been caught (see #8530). This PR does the following: * Makes `jacocorunner` attribute a `filegroup` instead of a `java_import` to avoid the dependency cycle. * Converts the type of `jacocoRunner` in `JavaToolchain` from a `TransitiveInfoCollection` to `FilesToRunProvider` to be able to retrieve the executable jar from the `filegroup`. * Adds testing with `--host_java_toolchain` of the java_tools built as head This is a blocker for the new java_tools release (https://github.com/bazelbuild/java_tools/issues/8). Closes #8529. PiperOrigin-RevId: 250876908 --- .../lib/rules/java/JavaCompilationHelper.java | 16 ++++++++++------ .../lib/rules/java/JavaTargetAttributes.java | 12 ++++++++++++ .../build/lib/rules/java/JavaToolchain.java | 3 ++- .../lib/rules/java/JavaToolchainProvider.java | 8 ++++---- src/test/shell/bazel/bazel_coverage_java_test.sh | 2 ++ src/test/shell/bazel/bazel_java_test.sh | 2 ++ tools/jdk/BUILD.java_tools | 7 ++++++- 7 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java index f313b41d53c5e6..7106896d3808f5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java @@ -269,13 +269,17 @@ public ImmutableList getBootclasspathOrDefault() { } public boolean addCoverageSupport() { - TransitiveInfoCollection jacocoRunner = javaToolchain.getJacocoRunner(); - if (jacocoRunner != null - && JavaInfo.getProvider(JavaCompilationArgsProvider.class, jacocoRunner) != null) { - addLibrariesToAttributes(ImmutableList.of(jacocoRunner)); - return true; + FilesToRunProvider jacocoRunner = javaToolchain.getJacocoRunner(); + if (jacocoRunner == null) { + return false; + } + Artifact jacocoRunnerJar = jacocoRunner.getExecutable(); + if (isStrict()) { + attributes.addDirectJar(jacocoRunnerJar); } - return false; + attributes.addCompileTimeClassPathEntry(jacocoRunnerJar); + attributes.addRuntimeClassPathEntry(jacocoRunnerJar); + return true; } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java index d07f3f7aa0cedb..1be3b020dea6a7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java @@ -158,6 +158,12 @@ public Builder addRuntimeClassPathEntries(NestedSet classPathEntries) return this; } + public Builder addCompileTimeClassPathEntry(Artifact entry) { + Preconditions.checkArgument(!built); + compileTimeClassPath.add(entry); + return this; + } + public Builder addCompileTimeClassPathEntries(NestedSet entries) { Preconditions.checkArgument(!built); compileTimeClassPath.addTransitive(entries); @@ -233,6 +239,12 @@ public Builder addDirectJars(NestedSet directJars) { return this; } + public Builder addDirectJar(Artifact directJar) { + Preconditions.checkArgument(!built); + this.directJars.add(directJar); + return this; + } + public Builder addCompileTimeDependencyArtifacts(NestedSet dependencyArtifacts) { Preconditions.checkArgument(!built); compileTimeDependencyArtifacts.addTransitive(dependencyArtifacts); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java index 79c4fe356fface..0139169948f701 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java @@ -106,7 +106,8 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext.getPrerequisites( "package_configuration", Mode.HOST, JavaPackageConfigurationProvider.class)); - TransitiveInfoCollection jacocoRunner = ruleContext.getPrerequisite("jacocorunner", Mode.HOST); + FilesToRunProvider jacocoRunner = + ruleContext.getExecutablePrerequisite("jacocorunner", Mode.HOST); JavaToolchainProvider provider = JavaToolchainProvider.create( diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java index a947a32f91c89e..343f2349d93f85 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java @@ -91,7 +91,7 @@ public static JavaToolchainProvider create( FilesToRunProvider ijar, ImmutableListMultimap compatibleJavacOptions, ImmutableList packageConfiguration, - TransitiveInfoCollection jacocoRunner, + FilesToRunProvider jacocoRunner, JavaSemantics javaSemantics) { return new JavaToolchainProvider( label, @@ -142,7 +142,7 @@ public static JavaToolchainProvider create( private final ImmutableList javabuilderJvmOptions; private final boolean javacSupportsWorkers; private final ImmutableList packageConfiguration; - private final TransitiveInfoCollection jacocoRunner; + private final FilesToRunProvider jacocoRunner; private final JavaSemantics javaSemantics; @VisibleForSerialization @@ -169,7 +169,7 @@ public static JavaToolchainProvider create( ImmutableList javabuilderJvmOptions, boolean javacSupportsWorkers, ImmutableList packageConfiguration, - TransitiveInfoCollection jacocoRunner, + FilesToRunProvider jacocoRunner, JavaSemantics javaSemantics) { super(ImmutableMap.of(), Location.BUILTIN); @@ -339,7 +339,7 @@ public ImmutableList packageConfiguration() { return packageConfiguration; } - public TransitiveInfoCollection getJacocoRunner() { + public FilesToRunProvider getJacocoRunner() { return jacocoRunner; } diff --git a/src/test/shell/bazel/bazel_coverage_java_test.sh b/src/test/shell/bazel/bazel_coverage_java_test.sh index 28c876c450204b..c2b741e9512184 100755 --- a/src/test/shell/bazel/bazel_coverage_java_test.sh +++ b/src/test/shell/bazel/bazel_coverage_java_test.sh @@ -23,6 +23,7 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \ JAVA_TOOLCHAIN="$1"; shift add_to_bazelrc "build --java_toolchain=${JAVA_TOOLCHAIN}" +add_to_bazelrc "build --host_java_toolchain=${JAVA_TOOLCHAIN}" JAVA_TOOLS_ZIP="$1"; shift if [[ "${JAVA_TOOLS_ZIP}" != "released" ]]; then @@ -37,6 +38,7 @@ JAVA_TOOLS_ZIP_FILE_URL=${JAVA_TOOLS_ZIP_FILE_URL:-} if [[ $# -gt 0 ]]; then JAVABASE_VALUE="$1"; shift add_to_bazelrc "build --javabase=${JAVABASE_VALUE}" + add_to_bazelrc "build --host_javabase=${JAVABASE_VALUE}" fi function set_up() { diff --git a/src/test/shell/bazel/bazel_java_test.sh b/src/test/shell/bazel/bazel_java_test.sh index 58d855173f91ea..a867f19ccffbce 100755 --- a/src/test/shell/bazel/bazel_java_test.sh +++ b/src/test/shell/bazel/bazel_java_test.sh @@ -57,6 +57,7 @@ fi JAVA_TOOLCHAIN="$1"; shift add_to_bazelrc "build --java_toolchain=${JAVA_TOOLCHAIN}" +add_to_bazelrc "build --host_java_toolchain=${JAVA_TOOLCHAIN}" JAVA_TOOLS_ZIP="$1"; shift if [[ "${JAVA_TOOLS_ZIP}" != "released" ]]; then @@ -73,6 +74,7 @@ JAVA_TOOLS_ZIP_FILE_URL=${JAVA_TOOLS_ZIP_FILE_URL:-} if [[ $# -gt 0 ]]; then JAVABASE_VALUE="$1"; shift add_to_bazelrc "build --javabase=${JAVABASE_VALUE}" + add_to_bazelrc "build --host_javabase=${JAVABASE_VALUE}" fi function set_up() { diff --git a/tools/jdk/BUILD.java_tools b/tools/jdk/BUILD.java_tools index 402e51da149844..ccb7a4e45944e0 100644 --- a/tools/jdk/BUILD.java_tools +++ b/tools/jdk/BUILD.java_tools @@ -59,7 +59,7 @@ java_toolchain( ":java_compiler_jar", ":jdk_compiler_jar", ], - jacocorunner = ":jacoco_coverage_runner" + jacocorunner = ":jacoco_coverage_runner_filegroup" ) filegroup( @@ -72,6 +72,11 @@ filegroup( srcs = ["java_tools/GenClass_deploy.jar"], ) +filegroup( + name = "jacoco_coverage_runner_filegroup", + srcs = ["java_tools/JacocoCoverage_jarjar_deploy.jar"], +) + java_import( name = "jacoco_coverage_runner", jars = ["java_tools/JacocoCoverage_jarjar_deploy.jar"],