Only fetch @remote_coverage_tools when collecting coverage

Before this change, every test rule had an implicit dependency on `@bazel_tools//tools/test:coverage_report_generator`, even though this tool is only used when collecting coverage. This is fixed by moving it to `CoverageOptions` and using the late-bound default resolver to only create a dependency if coverage is enabled. Also adds `CoverageOptions` to the set of options classes trimmed by `--trim_test_configuration` so that, as before, changing `--coverage_report_generator` doesn't cause non-test rules to be reanalyzed. This behavior now extends to `--coverage_output_generator`. Fixes #15088 Closes #16995. PiperOrigin-RevId: 498949871 Change-Id: I2440fae2655bbb701e918ee2aa7acb008d8f97ed
bazelbuild · Feb 14, 2023 · 0ab513e · 0ab513e
1 parent 23daf78
commit 0ab513e
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 34 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -252,7 +252,6 @@ java_library(
         "test/AnalysisTestActionBuilder.java",
         "test/BaselineCoverageAction.java",
         "test/CoverageCommon.java",
-        "test/CoverageConfiguration.java",
         "test/InstrumentedFileManifestAction.java",
         "test/InstrumentedFilesCollector.java",
         "test/TestActionBuilder.java",
@@ -360,6 +359,7 @@ java_library(
         ":test/analysis_failure_propagation_exception",
         ":test/analysis_test_result_info",
         ":test/baseline_coverage_result",
+        ":test/coverage_configuration",
         ":test/execution_info",
         ":test/instrumented_files_info",
         ":test/test_configuration",
@@ -2544,6 +2544,24 @@ java_library(
     ],
 )
 
+java_library(
+    name = "test/coverage_configuration",
+    srcs = ["test/CoverageConfiguration.java"],
+    deps = [
+        ":config/build_options",
+        ":config/core_option_converters",
+        ":config/core_options",
+        ":config/fragment",
+        ":config/fragment_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/concurrent",
+        "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
+        "//src/main/java/com/google/devtools/common/options",
+        "//third_party:jsr305",
+    ],
+)
+
 java_library(
     name = "test/coverage_report_action_factory",
     srcs = ["test/CoverageReportActionFactory.java"],
@@ -2595,6 +2613,7 @@ java_library(
         ":config/fragment_options",
         ":config/per_label_options",
         ":options_diff_predicate",
+        ":test/coverage_configuration",
         ":test/test_sharding_strategy",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -143,13 +143,16 @@ public static LabelLateBoundDefault<TestConfiguration> coverageSupportAttribute(
       "//tools/test:coverage_report_generator";
 
   @SerializationConstant @AutoCodec.VisibleForSerialization
-  static final Resolver<TestConfiguration, Label> COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER =
-      (rule, attributes, configuration) -> configuration.getCoverageReportGenerator();
+  static final Resolver<CoverageConfiguration, Label>
+      COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER =
+          (rule, attributes, configuration) -> configuration.reportGenerator();
 
-  public static LabelLateBoundDefault<TestConfiguration> coverageReportGeneratorAttribute(
+  public static LabelLateBoundDefault<CoverageConfiguration> coverageReportGeneratorAttribute(
       Label defaultValue) {
     return LabelLateBoundDefault.fromTargetConfiguration(
-        TestConfiguration.class, defaultValue, COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER);
+        CoverageConfiguration.class,
+        defaultValue,
+        COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER);
   }
 
   public static LabelLateBoundDefault<CoverageConfiguration> getCoverageOutputGeneratorLabel() {

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java
@@ -52,6 +52,22 @@ public static class CoverageOptions extends FragmentOptions {
                 + "currently be a filegroup that contains a single file, the binary. Defaults to "
                 + "'//tools/test:lcov_merger'.")
     public Label coverageOutputGenerator;
+
+    @Option(
+        name = "coverage_report_generator",
+        converter = LabelConverter.class,
+        defaultValue = "@bazel_tools//tools/test:coverage_report_generator",
+        documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
+        effectTags = {
+          OptionEffectTag.CHANGES_INPUTS,
+          OptionEffectTag.AFFECTS_OUTPUTS,
+          OptionEffectTag.LOADING_AND_ANALYSIS
+        },
+        help =
+            "Location of the binary that is used to generate coverage reports. This must "
+                + "currently be a filegroup that contains a single file, the binary. Defaults to "
+                + "'//tools/test:coverage_report_generator'.")
+    public Label coverageReportGenerator;
   }
 
   private final CoverageOptions coverageOptions;
@@ -75,4 +91,12 @@ public Label outputGenerator() {
     }
     return coverageOptions.coverageOutputGenerator;
   }
+
+  @Nullable
+  public Label reportGenerator() {
+    if (coverageOptions == null) {
+      return null;
+    }
+    return coverageOptions.coverageReportGenerator;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
+import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
 import com.google.devtools.build.lib.analysis.test.TestShardingStrategy.ShardingStrategyConverter;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.TestTimeout;
@@ -51,7 +52,9 @@ public class TestConfiguration extends Fragment {
           // changes in --trim_test_configuration itself or related flags always prompt invalidation
           return true;
         }
-        if (!changedOption.getField().getDeclaringClass().equals(TestOptions.class)) {
+        Class<?> affectedOptionsClass = changedOption.getField().getDeclaringClass();
+        if (!affectedOptionsClass.equals(TestOptions.class)
+            && !affectedOptionsClass.equals(CoverageOptions.class)) {
           // options outside of TestOptions always prompt invalidation
           return true;
         }
@@ -241,23 +244,6 @@ public static class TestOptions extends FragmentOptions {
     )
     public Label coverageSupport;
 
-    @Option(
-        name = "coverage_report_generator",
-        converter = LabelConverter.class,
-        defaultValue = "@bazel_tools//tools/test:coverage_report_generator",
-        documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
-        effectTags = {
-            OptionEffectTag.CHANGES_INPUTS,
-            OptionEffectTag.AFFECTS_OUTPUTS,
-            OptionEffectTag.LOADING_AND_ANALYSIS
-        },
-        help =
-            "Location of the binary that is used to generate coverage reports. This must "
-                + "currently be a filegroup that contains a single file, the binary. Defaults to "
-                + "'//tools/test:coverage_report_generator'."
-    )
-    public Label coverageReportGenerator;
-
     @Option(
         name = "experimental_fetch_all_coverage_outputs",
         defaultValue = "false",
@@ -358,10 +344,6 @@ public Label getCoverageSupport() {
     return options.coverageSupport;
   }
 
-  public Label getCoverageReportGenerator() {
-    return options.coverageReportGenerator;
-  }
-
   /**
    * @return number of times the given test should run. If the test doesn't match any of the
    *     filters, runs it once.

diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD
@@ -60,6 +60,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory",
         "//src/main/java/com/google/devtools/build/lib/cmdline",

diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
@@ -1836,8 +1836,9 @@ EOF
   fi
 }
 
-function test_cc_test_no_lcov_merger_dep_without_coverage() {
-  # Regression test for https://github.com/bazelbuild/bazel/issues/16961
+function test_cc_test_no_coverage_tools_dep_without_coverage() {
+  # Regression test for https://github.com/bazelbuild/bazel/issues/16961 and
+  # https://github.com/bazelbuild/bazel/issues/15088.
   local package="${FUNCNAME[0]}"
   mkdir -p "${package}"
 
@@ -1849,12 +1850,9 @@ cc_test(
 EOF
   touch "${package}"/test.cc
 
-  # FIXME: cc_test still unconditionally depends on the LCOV merger binary through
-  #  @remote_coverage_tools//:coverage_output_generator, which is also unnecessary:
-  #  https://github.com/bazelbuild/bazel/issues/15088
-  out=$(bazel cquery "somepath(//${package}:test,@remote_coverage_tools//:lcov_merger)")
+  out=$(bazel cquery "somepath(//${package}:test,@remote_coverage_tools//:all)")
   if [[ -n "$out" ]]; then
-    fail "Expected no dependency on lcov_merger, but got: $out"
+    fail "Expected no dependency on remote coverage tools, but got: $out"
   fi
 }