Java coverage: fix handling of external files

ulfjack · katre · commit 74de0ba4e793 · 2021-07-13T10:40:35.000-04:00
When java_test rules are run with coverage, then Bazel writes a file containing the runtime classpath using root-relative paths. The collect_coverage.sh script then processes that file to generate another file containing absolute paths by prefixing each of the paths with the runfiles directory and the workspace name. It then runs the singlejar tool on this list of jar files to merge them into a single file. This happens to work for all jar files in the main repository because the runfiles path is the runfiles directory plus the workspace name plus the root-relative path. However, it is broken if some of the jar files come from an external repository. It appears that singlejar errors out on the first such jar file, generating a partial output jar file without a central directory. The error is swallowed by the `collect_coverage.sh` script. Presence of coverage results may thus depend on the order of classpath entries. In order to fix this, we change the runtime classpath file to contain runfiles-relative paths, and the coverage script to prefix them with the runfiles directory. Note that we reuse SourceManifestAction here, i.e., the identical code that is also responsible for generating the runfiles directory in the first place. This is the only reliable way to get the correct paths into the test action. There are more places that use `LazyWritePathsFileAction` and I suspect that all of them are broken: - persistent test runner support in BazelJavaSemantics (AFAIK, this doesn't work in Bazel anyway) - coverage w/ metadata jars in BazelJavaSemantics (not used by JavaBinary) - coverage source list file ('-paths-for-coverage.txt'); this probably results in non-functioning coverage for *source files* in external repositories Fixes bazelbuild#13376. Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352 Closes bazelbuild#13377. Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352 PiperOrigin-RevId: 369687409
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
@@ -31,10 +31,11 @@
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
+import com.google.devtools.build.lib.analysis.SourceManifestAction;
+import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.analysis.actions.LazyWritePathsFileAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -429,19 +430,25 @@ public ConfiguredTarget create(RuleContext ruleContext)
     NestedSetBuilder<Artifact> coverageSupportFiles = NestedSetBuilder.stableOrder();
     if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
 
-      // Create an artifact that contains the root relative paths of the jars on the runtime
-      // classpath.
+      // Create an artifact that contains the runfiles relative paths of the jars on the runtime
+      // classpath. Using SourceManifestAction is the only reliable way to match the runfiles
+      // creation code.
       Artifact runtimeClasspathArtifact =
           ruleContext.getUniqueDirectoryArtifact(
               "runtime_classpath_for_coverage",
               "runtime_classpath.txt",
               ruleContext.getBinOrGenfilesDirectory());
       ruleContext.registerAction(
-          new LazyWritePathsFileAction(
+          new SourceManifestAction(
+              ManifestType.SOURCES_ONLY,
               ruleContext.getActionOwner(),
               runtimeClasspathArtifact,
-              common.getRuntimeClasspath(),
-              /* filesToIgnore= */ ImmutableSet.of(),
+              new Runfiles.Builder(
+                      ruleContext.getWorkspaceName(),
+                      ruleContext.getConfiguration().legacyExternalRunfiles())
+                  // This matches the code below in collectDefaultRunfiles.
+                  .addTransitiveArtifactsWrappedInStableOrder(common.getRuntimeClasspath())
+                  .build(),
               true));
       filesBuilder.add(runtimeClasspathArtifact);
 
diff --git a/tools/test/collect_coverage.sh b/tools/test/collect_coverage.sh
@@ -140,7 +140,7 @@ if [[ ! -z "${JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE}" ]]; then
   # Append the runfiles prefix to all the relative paths found in
   # JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE, to invoke SingleJar with the
   # absolute paths.
-  RUNFILES_PREFIX="$TEST_SRCDIR/$TEST_WORKSPACE/"
+  RUNFILES_PREFIX="$TEST_SRCDIR/"
   cat "$JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE" | sed "s@^@$RUNFILES_PREFIX@" >> "$single_jar_params_file"
 
   # Invoke SingleJar. This will create JACOCO_METADATA_JAR.
