From aec5d9261b113f4ddfda936ffab386134a9a97e9 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 12 Dec 2022 16:31:34 +0100 Subject: [PATCH] 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`. --- .../google/devtools/build/lib/analysis/BUILD | 21 ++++++++++++++- .../build/lib/analysis/BaseRuleClasses.java | 12 +++++---- .../analysis/test/CoverageConfiguration.java | 24 +++++++++++++++++ .../lib/analysis/test/TestConfiguration.java | 26 +++---------------- .../com/google/devtools/build/lib/rules/BUILD | 1 + src/test/shell/bazel/cc_integration_test.sh | 12 ++++----- 6 files changed, 61 insertions(+), 35 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 index 2211b2a98d11f2..6bf532e0cdcfad 100644 --- 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_checked_in", + ], +) + 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 index 2c5dd9c24a4b4a..7b76e5888b858b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -142,14 +142,16 @@ public static LabelLateBoundDefault coverageSupportAttribute( public static final String DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE = "//tools/test:coverage_report_generator"; - @SerializationConstant @AutoCodec.VisibleForSerialization - static final Resolver COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER = - (rule, attributes, configuration) -> configuration.getCoverageReportGenerator(); + @SerializationConstant + @AutoCodec.VisibleForSerialization + static final Resolver COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER = + (rule, attributes, configuration) -> configuration.reportGenerator(); - public static LabelLateBoundDefault coverageReportGeneratorAttribute( + public static LabelLateBoundDefault 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 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 index 7aaccfa4d4eec2..bf19796e41ba35 100644 --- 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,23 @@ 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 +92,11 @@ public Label outputGenerator() { } return coverageOptions.coverageOutputGenerator; } + + 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 index acea2dc00b3068..cfde8d02513c83 100644 --- 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 index cc565dd9cdf708..048e80c2aa73a6 100644 --- 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 index 982104158d1388..f411a15f1f16aa 100755 --- 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 }