Pass the name of the classpath manifest jar to JacocoCoverageRunner

When the java classpath exceeds the limit, we create a manifest JAR
and pass that on the classpath. JacocoCoverageRunner knows how to
extract information from this JAR. But if another JAR ends up on that
classpath, it confuses the coverage runner which is expecting only a
single jar in this case.

This changes the java_stub_template file to export the name of the
created manifest jar so the coverage runner can extract it.

We also change the template file so the relevant exports don't occur
in the middle of a larger function.

It's possible this is somewhat overengineered and that we could
always rely on the manifest jar always being the first one discovered
by the coverage runner, but it is not totally obvious to me that that
will always be true.

Fixes bazelbuild#21268

Closes bazelbuild#21365.

PiperOrigin-RevId: 608333782
Change-Id: I9895689fd9d771c9198e36bef222a9f86ada573e

src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java

@@ -20,6 +20,7 @@
 import static java.nio.file.StandardOpenOption.CREATE;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableList.Builder;
 import com.google.common.collect.ImmutableSet;
@@ -360,15 +361,33 @@ private static ImmutableList<File> getFilesFromFileList(File mainFile, String ja
     return convertedMetadataFiles.build();
   }
 
-  private static URL[] getUrls(ClassLoader classLoader, boolean wasWrappedJar) {
+  private static URL[] getUrls(ClassLoader classLoader, boolean jarIsWrapped, String wrappedJar) {
+    // jarIsWrapped is a legacy parameter; it should be removed once we are sure Bazel will no
+    // longer set JACOCO_IS_JAR_WRAPPED in java_stub_template
     URL[] urls = getClassLoaderUrls(classLoader);
+    if (urls == null || urls.length == 0) {
+      return urls;
+    }
     // If the classpath was too long then a temporary top-level jar is created containing nothing
-    // but a manifest with
-    // the original classpath. Those are the URLs we are looking for.
-    if (wasWrappedJar && urls != null && urls.length == 1) {
+    // but a manifest with the original classpath. Those are the URLs we are looking for.
+    URL classPathUrl = null;
+    if (!Strings.isNullOrEmpty(wrappedJar)) {
+      for (URL url : urls) {
+        if (url.getPath().endsWith(wrappedJar)) {
+          classPathUrl = url;
+        }
+      }
+      if (classPathUrl == null) {
+        System.err.println("Classpath JAR " + wrappedJar + " not provided");
+        return null;
+      }
+    } else if (jarIsWrapped && urls.length == 1) {
+      classPathUrl = urls[0];
+    }
+    if (classPathUrl != null) {
       try {
         String jarClassPath =
-            new JarInputStream(urls[0].openStream())
+            new JarInputStream(classPathUrl.openStream())
                 .getManifest()
                 .getMainAttributes()
                 .getValue("Class-Path");
@@ -428,14 +447,15 @@ private static URL[] getClassLoaderUrls(ClassLoader classLoader) {
   public static void main(String[] args) throws Exception {
     String metadataFile = System.getenv("JACOCO_METADATA_JAR");
     String jarWrappedValue = System.getenv("JACOCO_IS_JAR_WRAPPED");
+    String wrappedJarValue = System.getenv("CLASSPATH_JAR");
     boolean wasWrappedJar = jarWrappedValue != null ? !jarWrappedValue.equals("0") : false;
 
     File[] metadataFiles = null;
     int deployJars = 0;
     final HashMap<String, byte[]> uninstrumentedClasses = new HashMap<>();
     ImmutableSet.Builder<String> pathsForCoverageBuilder = new ImmutableSet.Builder<>();
     ClassLoader classLoader = ClassLoader.getSystemClassLoader();
-    URL[] urls = getUrls(classLoader, wasWrappedJar);
+    URL[] urls = getUrls(classLoader, wasWrappedJar, wrappedJarValue);
     if (urls != null) {
       metadataFiles = new File[urls.length];
       for (int i = 0; i < urls.length; i++) {

src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt

@@ -269,7 +269,11 @@ export CLASSLOADER_PREFIX_PATH="${RUNPATH}"
 %set_jacoco_metadata%
 %set_jacoco_main_class%
 %set_jacoco_java_runfiles_root%
+# export JACOCO_IS_JAR_WRAPPED for compatibility with older versions of
+# JacocoCoverageRunner that check for this and not CLASSPATH_JAR
+# TODO(cmita): Remove when this is no longer required
 export JACOCO_IS_JAR_WRAPPED=0
+export CLASSPATH_JAR=""
 
 if [[ -n "$JVM_DEBUG_PORT" ]]; then
   JVM_DEBUG_SUSPEND=${DEFAULT_JVM_DEBUG_SUSPEND:-"y"}
@@ -295,7 +299,8 @@ ARGS=(
   "${ARGS[@]}")
 
 
-function create_and_run_classpath_jar() {
+# Creates a JAR containing the classpath and put the result to stdout
+function create_classpath_jar() {
   # Build class path.
   MANIFEST_CLASSPATH=()
   if is_windows; then
@@ -365,13 +370,9 @@ function create_and_run_classpath_jar() {
   fi
   $JARBIN cvfm "$MANIFEST_JAR_FILE" "$MANIFEST_FILE" >/dev/null || \
     die "ERROR: $self failed because $JARBIN failed"
-
-  # Execute JAVA command
-  $JAVABIN -classpath "$MANIFEST_JAR_FILE" "${ARGS[@]}"
-  exit_code=$?
   rm -f "$MANIFEST_FILE"
-  rm -f "$MANIFEST_JAR_FILE"
-  exit $exit_code
+
+  echo "$MANIFEST_JAR_FILE"
 }
 
 # If the user didn't specify a --classpath_limit, use the default value.
@@ -393,8 +394,17 @@ if ! is_macos; then
 fi
 
 if (("${#CLASSPATH}" > ${CLASSPATH_LIMIT})); then
+  # TODO(cmtia): Remove JACOCO_IS_JAR_WRAPPED when JacocoCoverageRunner will
+  # never need it anymore.
   export JACOCO_IS_JAR_WRAPPED=1
-  create_and_run_classpath_jar
+  CLASSPATH_MANIFEST_JAR=$(create_classpath_jar)
+  export CLASSPATH_JAR="$(basename $CLASSPATH_MANIFEST_JAR)"
+  $JAVABIN -classpath "$CLASSPATH_MANIFEST_JAR" "${ARGS[@]}"
+  exit_code=$?
+  rm -f "$CLASSPATH_MANIFEST_JAR"
+  exit $exit_code
 else
+  export JACOCO_IS_JAR_WRAPPED=0
+  export CLASSPATH_JAR=""
   exec $JAVABIN -classpath $CLASSPATH "${ARGS[@]}"
 fi

src/test/shell/bazel/bazel_coverage_java_test.sh

@@ -632,6 +632,171 @@ end_of_record"
   assert_coverage_result "$expected_result_random" ${coverage_file_path}
 }
 
+function test_java_coverage_with_classpath_jar() {
+  # Verifies the logic in JacocoCoverageRunner can unpack the classpath jar
+  # created when the classpath is too long.
+  cat <<EOF > BUILD
+java_library(
+    name = "lib",
+    srcs = ["src/main/java/lib/Lib.java"],
+)
+
+java_test(
+    name = "lib_test",
+    srcs = ["src/test/java/lib/TestLib.java"],
+    test_class = "lib.TestLib",
+    deps = [":lib"],
+)
+EOF
+
+  mkdir -p src/main/java/lib
+  cat <<EOF > src/main/java/lib/Lib.java
+package lib;
+public class Lib {
+  public static int calcX(int y) {
+    return y * 2;
+  }
+}
+EOF
+
+  mkdir -p src/test/java/lib
+  cat <<EOF > src/test/java/lib/TestLib.java
+package lib;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Test;
+
+public class TestLib {
+  @Test
+  public void testCalcX() throws Exception {
+    assertEquals(6, Lib.calcX(3));
+  }
+}
+EOF
+
+  bazel coverage \
+    --test_output=all \
+    --combined_report=lcov \
+    --instrumentation_filter="lib" \
+    --action_env CLASSPATH_LIMIT=1 \
+    //:lib_test &>$TEST_log \
+      || echo "Coverage for //:test failed"
+
+  local coverage_file_path="$(get_coverage_file_path_from_test_log)"
+  local expected_result="SF:src/main/java/lib/Lib.java
+FN:2,lib/Lib::<init> ()V
+FN:4,lib/Lib::calcX (I)I
+FNDA:0,lib/Lib::<init> ()V
+FNDA:1,lib/Lib::calcX (I)I
+FNF:2
+FNH:1
+DA:2,0
+DA:4,1
+LH:1
+LF:2"
+
+  assert_coverage_result "$expected_result" "$coverage_file_path"
+}
+
+function test_java_coverage_with_classpath_and_data_jar() {
+  # Ignore this test when testing the released java tools.
+  # TODO(cmita): Enable the test in this case after a java_tools release
+  if [[ "$JAVA_TOOLS_ZIP" == "released" ]]; then
+    return 0
+  fi
+  cat <<EOF > BUILD
+java_binary(
+    name = "foo",
+    srcs = ["src/main/java/foo/Foo.java"],
+    main_class = "foo.Foo",
+    deploy_manifest_lines = [
+      "Premain-Class: foo.Foo",
+    ],
+    use_launcher = False,
+)
+
+java_library(
+    name = "lib",
+    srcs = ["src/main/java/lib/Lib.java"],
+)
+
+java_test(
+    name = "lib_test",
+    srcs = ["src/test/java/lib/TestLib.java"],
+    test_class = "lib.TestLib",
+    deps = [":lib"],
+    jvm_flags = ["-javaagent:\$(rootpath :foo_deploy.jar)"],
+    data = [":foo_deploy.jar"],
+)
+EOF
+
+  mkdir -p src/main/java/foo
+  cat <<EOF > src/main/java/foo/Foo.java
+package foo;
+public class Foo {
+  public static void main(String[] args) {
+    return;
+  }
+
+  public static void premain(String args) {
+    return;
+  }
+
+  public static void agentmain(String args) {
+    return;
+  }
+}
+EOF
+
+  mkdir -p src/main/java/lib
+  cat <<EOF > src/main/java/lib/Lib.java
+package lib;
+public class Lib {
+  public static int calcX(int y) {
+    return y * 2;
+  }
+}
+EOF
+
+  mkdir -p src/test/java/lib
+  cat <<EOF > src/test/java/lib/TestLib.java
+package lib;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Test;
+
+public class TestLib {
+  @Test
+  public void testCalcX() throws Exception {
+    assertEquals(6, Lib.calcX(3));
+  }
+}
+EOF
+
+  bazel coverage \
+    --test_output=all \
+    --combined_report=lcov \
+    --instrumentation_filter="lib" \
+    --action_env CLASSPATH_LIMIT=1 \
+    //:lib_test &>$TEST_log \
+      || echo "Coverage for //:test failed"
+
+  local coverage_file_path="$(get_coverage_file_path_from_test_log)"
+  local expected_result="SF:src/main/java/lib/Lib.java
+FN:2,lib/Lib::<init> ()V
+FN:4,lib/Lib::calcX (I)I
+FNDA:0,lib/Lib::<init> ()V
+FNDA:1,lib/Lib::calcX (I)I
+FNF:2
+FNH:1
+DA:2,0
+DA:4,1
+LH:1
+LF:2"
+
+  assert_coverage_result "$expected_result" "$coverage_file_path"
+}
+
 function test_java_string_switch_coverage() {
   # Verify that Jacoco's filtering is being applied.
   # Switches on strings generate over double the number of expected branches