From 4a753499d0685ce08c99c1da61eea4e100989d31 Mon Sep 17 00:00:00 2001 From: Yue Gan Date: Tue, 20 Dec 2016 04:54:55 +0000 Subject: [PATCH] Avoids NullPointerException when running `bazel coverage //:xxx`, since PathFragment.getParentDirectory() may return null. Fixes #2212. Also adds coverage-specific attributes to the java_binary rule. -- PiperOrigin-RevId: 142516883 MOS_MIGRATED_REVID=142516883 --- .../bazel/rules/java/BazelJavaBinaryRule.java | 7 ++ .../bazel/rules/java/BazelJavaSemantics.java | 80 +++++++++---------- .../bazel/rules/java/BazelJavaTestRule.java | 4 +- .../lib/runtime/commands/CoverageCommand.java | 17 ++-- src/test/shell/bazel/bazel_coverage_test.sh | 17 ++-- tools/jdk/BUILD | 11 +++ 6 files changed, 78 insertions(+), 58 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaBinaryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaBinaryRule.java index 5cd254c6b09656..d414fa149435a3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaBinaryRule.java +++ b/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(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java index 4c14c73b5bac13..ecaf32e23e7ec5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java +++ b/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 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 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 diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaTestRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaTestRule.java index ac294bd3d90594..dfc26a60199d11 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaTestRule.java +++ b/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"))) /* The Java class to be loaded by the test runner.

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java index 186bcd47c0e204..4b1471b9d23cb0 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java @@ -243,7 +243,10 @@ private void collectInstrumentedPackages(CommandEnvironment env, Collection targets, Set 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,7 +293,7 @@ public static String getInstrumentedPrefix(String packageName) { .replaceFirst("(?<=^|/)test/java/", "main/java/"); } - private void optimizeFilterSet(SortedSet packageFilters) { + private static void optimizeFilterSet(SortedSet packageFilters) { Iterator iterator = packageFilters.iterator(); if (iterator.hasNext()) { // Find common parent filters to reduce number of filter expressions. In practice this @@ -298,15 +301,15 @@ private void optimizeFilterSet(SortedSet packageFilters) { // filter value much more user-friendly - especially in case of /my/package/... wildcards. Set 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); diff --git a/src/test/shell/bazel/bazel_coverage_test.sh b/src/test/shell/bazel/bazel_coverage_test.sh index 2fcd25f92dbf8b..9fd23ca6a6ea02 100755 --- a/src/test/shell/bazel/bazel_coverage_test.sh +++ b/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 < java_test/BUILD + cat < 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 < java_test/src/main/com/example/Collatz.java + mkdir -p src/main/com/example + cat < 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 < java_test/src/test/com/example/TestCollatz.java + mkdir -p src/test/com/example + cat < 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 < java_test/result.dat + cat < result.dat SF:com/example/Collatz.java FN:3,com/example/Collatz:: ()V FNDA:0,com/example/Collatz:: ()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 } diff --git a/tools/jdk/BUILD b/tools/jdk/BUILD index 22d26eba9b85c0..5b5596b1184fc8 100644 --- a/tools/jdk/BUILD +++ b/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", +)