Avoids NullPointerException when running bazel coverage //:xxx, sin…

…ce PathFragment.getParentDirectory() may return null. Fixes #2212.

Also adds coverage-specific attributes to the java_binary rule.

--
PiperOrigin-RevId: 142516883
MOS_MIGRATED_REVID=142516883

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.bazel.rules.java;
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
 
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
@@ -77,6 +78,12 @@ public Object getDefault(AttributeMap rule) {
                 return rule.get("create_executable", BOOLEAN);
               }
             }))
+        .add(
+            attr("$jacoco_runtime", LABEL)
+                .value(env.getToolsLabel("//tools/jdk:jacoco-blaze-agent")))
+        .add(
+            attr("$jacocorunner", LABEL).value(env.getToolsLabel(
+                "//tools/jdk:JacocoCoverage")))
         .build();
   }
 

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

@@ -449,48 +449,48 @@ public String addCoverageSupport(
 
     if (!hasInstrumentationMetadata(attributes)) {
       return mainClass;
-    } else {
-      Artifact instrumentedJar =
-          helper
-              .getRuleContext()
-              .getBinArtifact(helper.getRuleContext().getLabel().getName() + "_instrumented.jar");
-
-      // Create an instrumented Jar. This will be referenced on the runtime classpath prior
-      // to all other Jars.
-      JavaCommon.createInstrumentedJarAction(
-          helper.getRuleContext(),
-          this,
-          attributes.getInstrumentationMetadata(),
-          instrumentedJar,
-          mainClass);
-      javaArtifactsBuilder.setInstrumentedJar(instrumentedJar);
-
-      // Add the coverage runner to the list of dependencies when compiling in coverage mode.
-      TransitiveInfoCollection runnerTarget =
-          helper.getRuleContext().getPrerequisite("$jacocorunner", Mode.TARGET);
-      if (runnerTarget.getProvider(JavaCompilationArgsProvider.class) != null) {
-        helper.addLibrariesToAttributes(ImmutableList.of(runnerTarget));
-      } else {
+    }
+
+    Artifact instrumentedJar =
         helper
             .getRuleContext()
-            .ruleError(
-                "this rule depends on "
-                    + helper.getRuleContext().attributes().get("$jacocorunner", BuildType.LABEL)
-                    + " which is not a java_library rule, or contains errors");
-      }
-      // In offline instrumentation mode, add the Jacoco runtime to the classpath as well (this
-      // jar is not included by the coverage runner). Note that $jacoco is provided via a
-      // filegroup because the same jar can be used for online instrumentation, by simply adding
-      // it to -javaagent and -Xbootclasspath/p (similar to the Reverifier setup). The $jacoco jar
-      // has a "Premain-Class:" entry in its manifest, which would get erased by ijar filtering,
-      // hence the filegroup.
-      TransitiveInfoCollection agentTarget =
-          helper.getRuleContext().getPrerequisite("$jacoco_runtime", Mode.TARGET);
-      NestedSet<Artifact> filesToBuild =
-          agentTarget.getProvider(FileProvider.class).getFilesToBuild();
-      for (Artifact jar : FileType.filter(filesToBuild, JavaSemantics.JAR)) {
-        attributes.addRuntimeClassPathEntry(jar);
-      }
+            .getBinArtifact(helper.getRuleContext().getLabel().getName() + "_instrumented.jar");
+
+    // Create an instrumented Jar. This will be referenced on the runtime classpath prior
+    // to all other Jars.
+    JavaCommon.createInstrumentedJarAction(
+        helper.getRuleContext(),
+        this,
+        attributes.getInstrumentationMetadata(),
+        instrumentedJar,
+        mainClass);
+    javaArtifactsBuilder.setInstrumentedJar(instrumentedJar);
+
+    // Add the coverage runner to the list of dependencies when compiling in coverage mode.
+    TransitiveInfoCollection runnerTarget =
+        helper.getRuleContext().getPrerequisite("$jacocorunner", Mode.TARGET);
+    if (runnerTarget.getProvider(JavaCompilationArgsProvider.class) != null) {
+      helper.addLibrariesToAttributes(ImmutableList.of(runnerTarget));
+    } else {
+      helper
+          .getRuleContext()
+          .ruleError(
+              "this rule depends on "
+                  + helper.getRuleContext().attributes().get("$jacocorunner", BuildType.LABEL)
+                  + " which is not a java_library rule, or contains errors");
+    }
+    // In offline instrumentation mode, add the Jacoco runtime to the classpath as well (this
+    // jar is not included by the coverage runner). Note that $jacoco is provided via a
+    // filegroup because the same jar can be used for online instrumentation, by simply adding
+    // it to -javaagent and -Xbootclasspath/p (similar to the Reverifier setup). The $jacoco jar
+    // has a "Premain-Class:" entry in its manifest, which would get erased by ijar filtering,
+    // hence the filegroup.
+    TransitiveInfoCollection agentTarget =
+        helper.getRuleContext().getPrerequisite("$jacoco_runtime", Mode.TARGET);
+    NestedSet<Artifact> filesToBuild =
+        agentTarget.getProvider(FileProvider.class).getFilesToBuild();
+    for (Artifact jar : FileType.filter(filesToBuild, JavaSemantics.JAR)) {
+      attributes.addRuntimeClassPathEntry(jar);
     }
 
     // We do not add the instrumented jar to the runtime classpath, but provide it in the shell

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

@@ -65,12 +65,12 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
         // Used in the one-per-build coverage report generation action.
         .add(
             attr("$jacoco_runtime", LABEL)
-                .value(env.getLabel("@bazel_tools//third_party/java/jacoco:blaze-agent")))
+                .value(env.getLabel("@bazel_tools//tools/jdk:jacoco-blaze-agent")))
         .add(
             attr("$jacocorunner", LABEL)
                 .value(
                     env.getLabel(
-                        "@bazel_tools//src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage")))
+                        "@bazel_tools//tools/jdk:JacocoCoverage")))
         /* <!-- #BLAZE_RULE(java_test).ATTRIBUTE(test_class) -->
         The Java class to be loaded by the test runner.<br/>
         <p>

src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java

@@ -243,7 +243,10 @@ private void collectInstrumentedPackages(CommandEnvironment env,
       Collection<Target> targets, Set<String> packageFilters) throws InterruptedException {
     for (Target target : targets) {
       // Add package-based filters for every test target.
-      packageFilters.add(getInstrumentedPrefix(target.getLabel().getPackageName()));
+      String prefix = getInstrumentedPrefix(target.getLabel().getPackageName());
+      if (!prefix.isEmpty()) {
+        packageFilters.add(prefix);
+      }
       if (TargetUtils.isTestSuiteRule(target)) {
         AttributeMap attributes = NonconfigurableAttributeMapper.of((Rule) target);
         // We don't need to handle $implicit_tests attribute since we already added
@@ -290,23 +293,23 @@ public static String getInstrumentedPrefix(String packageName) {
         .replaceFirst("(?<=^|/)test/java/", "main/java/");
   }
 
-  private void optimizeFilterSet(SortedSet<String> packageFilters) {
+  private static void optimizeFilterSet(SortedSet<String> packageFilters) {
     Iterator<String> iterator = packageFilters.iterator();
     if (iterator.hasNext()) {
       // Find common parent filters to reduce number of filter expressions. In practice this
       // still produces nicely constrained instrumentation filter while making final
       // filter value much more user-friendly - especially in case of /my/package/... wildcards.
       Set<String> parentFilters = Sets.newTreeSet();
       String filterString = iterator.next();
-      String parent = new PathFragment(filterString).getParentDirectory().getPathString();
+      PathFragment parent = new PathFragment(filterString).getParentDirectory();
       while (iterator.hasNext()) {
         String current = iterator.next();
-        if (parent != null && parent.length() > 0 &&
-            !current.startsWith(filterString) && current.startsWith(parent)) {
-          parentFilters.add(parent);
+        if (parent != null && parent.getPathString().length() > 0
+            && !current.startsWith(filterString) && current.startsWith(parent.getPathString())) {
+          parentFilters.add(parent.getPathString());
         } else {
           filterString = current;
-          parent = new PathFragment(filterString).getParentDirectory().getPathString();
+          parent = new PathFragment(filterString).getParentDirectory();
         }
       }
       packageFilters.addAll(parentFilters);

src/test/shell/bazel/bazel_coverage_test.sh

@@ -23,9 +23,8 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \
 
 # TODO(#2227): Re-enable when the jacoco processor issue is fixed.
 function DISABLED_test_java_test_coverage() {
-  mkdir java_test
 
-  cat <<EOF > java_test/BUILD
+  cat <<EOF > BUILD
 java_test(
     name = "test",
     srcs = glob(["src/test/**/*.java"]),
@@ -39,8 +38,8 @@ java_library(
 )
 EOF
 
-  mkdir -p java_test/src/main/com/example
-  cat <<EOF > java_test/src/main/com/example/Collatz.java
+  mkdir -p src/main/com/example
+  cat <<EOF > src/main/com/example/Collatz.java
 package com.example;
 
 public class Collatz {
@@ -59,8 +58,8 @@ public class Collatz {
 }
 EOF
 
-  mkdir -p java_test/src/test/com/example
-  cat <<EOF > java_test/src/test/com/example/TestCollatz.java
+  mkdir -p src/test/com/example
+  cat <<EOF > src/test/com/example/TestCollatz.java
 package com.example;
 
 import static org.junit.Assert.assertEquals;
@@ -79,13 +78,13 @@ public class TestCollatz {
 }
 EOF
 
-  bazel coverage //java_test:test &>$TEST_log || fail "Coverage for //java_test:test failed"
+  bazel coverage //:test &>$TEST_log || fail "Coverage for //:test failed"
 
   ending_part=$(sed -n -e '/PASSED/,$p' $TEST_log)
   coverage_file_path=$(grep -Eo "/[/a-zA-Z0-9\.\_\-]+\.dat$" <<< "$ending_part")
   [ -e $coverage_file_path ] || fail "Coverage output file not exists!"
 
-  cat <<EOF > java_test/result.dat
+  cat <<EOF > result.dat
 SF:com/example/Collatz.java
 FN:3,com/example/Collatz::<init> ()V
 FNDA:0,com/example/Collatz::<init> ()V
@@ -104,7 +103,7 @@ DA:12,7
 end_of_record
 EOF
 
-  if ! cmp java_test/result.dat $coverage_file_path; then
+  if ! cmp result.dat $coverage_file_path; then
     fail "Coverage output file is different with expected"
   fi
 }

tools/jdk/BUILD

@@ -155,3 +155,14 @@ py_test(
         ":proguard_whitelister",
     ],
 )
+
+# For java coverage
+alias(
+    name = "jacoco-blaze-agent",
+    actual = "//third_party/java/jacoco:blaze-agent",
+)
+
+alias(
+    name = "JacocoCoverage",
+    actual = "//src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage",
+)