From 1a438b41b74d94fd8b6ff4dcf20b4526530e3c6e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 19 Feb 2023 03:56:16 +0100 Subject: [PATCH] Only fetch @remote_coverage_tools when collecting coverage (#17512) 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 Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Co-authored-by: keertk <110264242+keertk@users.noreply.github.com> --- .../google/devtools/build/lib/analysis/BUILD | 21 ++++++++++++++- .../build/lib/analysis/BaseRuleClasses.java | 11 +++++--- .../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(+), 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 index b7dd37dce0f342..350541aee2ad63 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", @@ -2527,6 +2527,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", srcs = ["test/CoverageReport.java"], @@ -2588,6 +2606,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 bc14130e9cb3fb..7f4460242e7e20 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 @@ -143,13 +143,16 @@ public static LabelLateBoundDefault coverageSupportAttribute( "//tools/test:coverage_report_generator"; @SerializationConstant @AutoCodec.VisibleForSerialization - static final Resolver COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER = - (rule, attributes, configuration) -> configuration.getCoverageReportGenerator(); + 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..3825f0f657d2ec 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,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 index 7ff420cd9001d8..7da18906591961 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; } @@ -240,23 +243,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", @@ -357,10 +343,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 630f65a4d016c5..2a256ca6b2e02a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -59,6 +59,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//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: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 c02c27a0ba2997..93e653bddf967a 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/src/test/shell/bazel/cc_integration_test.sh @@ -1772,8 +1772,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}" @@ -1785,12 +1786,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 }