Skip to content

Commit 1a438b4

Browse files
fmeumShreeM01keertk
authored
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 <[email protected]> Co-authored-by: keertk <[email protected]>
1 parent a0fa77c commit 1a438b4

File tree

6 files changed

+61
-34
lines changed

6 files changed

+61
-34
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,6 @@ java_library(
252252
"test/AnalysisTestActionBuilder.java",
253253
"test/BaselineCoverageAction.java",
254254
"test/CoverageCommon.java",
255-
"test/CoverageConfiguration.java",
256255
"test/InstrumentedFileManifestAction.java",
257256
"test/InstrumentedFilesCollector.java",
258257
"test/TestActionBuilder.java",
@@ -360,6 +359,7 @@ java_library(
360359
":test/analysis_failure_propagation_exception",
361360
":test/analysis_test_result_info",
362361
":test/baseline_coverage_result",
362+
":test/coverage_configuration",
363363
":test/execution_info",
364364
":test/instrumented_files_info",
365365
":test/test_configuration",
@@ -2527,6 +2527,24 @@ java_library(
25272527
],
25282528
)
25292529

2530+
java_library(
2531+
name = "test/coverage_configuration",
2532+
srcs = ["test/CoverageConfiguration.java"],
2533+
deps = [
2534+
":config/build_options",
2535+
":config/core_option_converters",
2536+
":config/core_options",
2537+
":config/fragment",
2538+
":config/fragment_options",
2539+
"//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations",
2540+
"//src/main/java/com/google/devtools/build/lib/cmdline",
2541+
"//src/main/java/com/google/devtools/build/lib/concurrent",
2542+
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
2543+
"//src/main/java/com/google/devtools/common/options",
2544+
"//third_party:jsr305",
2545+
],
2546+
)
2547+
25302548
java_library(
25312549
name = "test/coverage_report",
25322550
srcs = ["test/CoverageReport.java"],
@@ -2588,6 +2606,7 @@ java_library(
25882606
":config/fragment_options",
25892607
":config/per_label_options",
25902608
":options_diff_predicate",
2609+
":test/coverage_configuration",
25912610
":test/test_sharding_strategy",
25922611
"//src/main/java/com/google/devtools/build/lib/cmdline",
25932612
"//src/main/java/com/google/devtools/build/lib/packages",

‎src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,13 +143,16 @@ public static LabelLateBoundDefault<TestConfiguration> coverageSupportAttribute(
143143
"//tools/test:coverage_report_generator";
144144

145145
@SerializationConstant @AutoCodec.VisibleForSerialization
146-
static final Resolver<TestConfiguration, Label> COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER =
147-
(rule, attributes, configuration) -> configuration.getCoverageReportGenerator();
146+
static final Resolver<CoverageConfiguration, Label>
147+
COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER =
148+
(rule, attributes, configuration) -> configuration.reportGenerator();
148149

149-
public static LabelLateBoundDefault<TestConfiguration> coverageReportGeneratorAttribute(
150+
public static LabelLateBoundDefault<CoverageConfiguration> coverageReportGeneratorAttribute(
150151
Label defaultValue) {
151152
return LabelLateBoundDefault.fromTargetConfiguration(
152-
TestConfiguration.class, defaultValue, COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER);
153+
CoverageConfiguration.class,
154+
defaultValue,
155+
COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER);
153156
}
154157

155158
public static LabelLateBoundDefault<CoverageConfiguration> getCoverageOutputGeneratorLabel() {

‎src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,22 @@ public static class CoverageOptions extends FragmentOptions {
5252
+ "currently be a filegroup that contains a single file, the binary. Defaults to "
5353
+ "'//tools/test:lcov_merger'.")
5454
public Label coverageOutputGenerator;
55+
56+
@Option(
57+
name = "coverage_report_generator",
58+
converter = LabelConverter.class,
59+
defaultValue = "@bazel_tools//tools/test:coverage_report_generator",
60+
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
61+
effectTags = {
62+
OptionEffectTag.CHANGES_INPUTS,
63+
OptionEffectTag.AFFECTS_OUTPUTS,
64+
OptionEffectTag.LOADING_AND_ANALYSIS
65+
},
66+
help =
67+
"Location of the binary that is used to generate coverage reports. This must "
68+
+ "currently be a filegroup that contains a single file, the binary. Defaults to "
69+
+ "'//tools/test:coverage_report_generator'.")
70+
public Label coverageReportGenerator;
5571
}
5672

5773
private final CoverageOptions coverageOptions;
@@ -75,4 +91,12 @@ public Label outputGenerator() {
7591
}
7692
return coverageOptions.coverageOutputGenerator;
7793
}
94+
95+
@Nullable
96+
public Label reportGenerator() {
97+
if (coverageOptions == null) {
98+
return null;
99+
}
100+
return coverageOptions.coverageReportGenerator;
101+
}
78102
}

‎src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
2626
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
2727
import com.google.devtools.build.lib.analysis.config.RequiresOptions;
28+
import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
2829
import com.google.devtools.build.lib.analysis.test.TestShardingStrategy.ShardingStrategyConverter;
2930
import com.google.devtools.build.lib.cmdline.Label;
3031
import com.google.devtools.build.lib.packages.TestTimeout;
@@ -51,7 +52,9 @@ public class TestConfiguration extends Fragment {
5152
// changes in --trim_test_configuration itself or related flags always prompt invalidation
5253
return true;
5354
}
54-
if (!changedOption.getField().getDeclaringClass().equals(TestOptions.class)) {
55+
Class<?> affectedOptionsClass = changedOption.getField().getDeclaringClass();
56+
if (!affectedOptionsClass.equals(TestOptions.class)
57+
&& !affectedOptionsClass.equals(CoverageOptions.class)) {
5558
// options outside of TestOptions always prompt invalidation
5659
return true;
5760
}
@@ -240,23 +243,6 @@ public static class TestOptions extends FragmentOptions {
240243
)
241244
public Label coverageSupport;
242245

243-
@Option(
244-
name = "coverage_report_generator",
245-
converter = LabelConverter.class,
246-
defaultValue = "@bazel_tools//tools/test:coverage_report_generator",
247-
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
248-
effectTags = {
249-
OptionEffectTag.CHANGES_INPUTS,
250-
OptionEffectTag.AFFECTS_OUTPUTS,
251-
OptionEffectTag.LOADING_AND_ANALYSIS
252-
},
253-
help =
254-
"Location of the binary that is used to generate coverage reports. This must "
255-
+ "currently be a filegroup that contains a single file, the binary. Defaults to "
256-
+ "'//tools/test:coverage_report_generator'."
257-
)
258-
public Label coverageReportGenerator;
259-
260246
@Option(
261247
name = "experimental_fetch_all_coverage_outputs",
262248
defaultValue = "false",
@@ -357,10 +343,6 @@ public Label getCoverageSupport() {
357343
return options.coverageSupport;
358344
}
359345

360-
public Label getCoverageReportGenerator() {
361-
return options.coverageReportGenerator;
362-
}
363-
364346
/**
365347
* @return number of times the given test should run. If the test doesn't match any of the
366348
* filters, runs it once.

‎src/main/java/com/google/devtools/build/lib/rules/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ java_library(
5959
"//src/main/java/com/google/devtools/build/lib/actions",
6060
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
6161
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
62+
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_configuration",
6263
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
6364
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory",
6465
"//src/main/java/com/google/devtools/build/lib/cmdline",

‎src/test/shell/bazel/cc_integration_test.sh‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1772,8 +1772,9 @@ EOF
17721772
fi
17731773
}
17741774

1775-
function test_cc_test_no_lcov_merger_dep_without_coverage() {
1776-
# Regression test for https://github.com/bazelbuild/bazel/issues/16961
1775+
function test_cc_test_no_coverage_tools_dep_without_coverage() {
1776+
# Regression test for https://github.com/bazelbuild/bazel/issues/16961 and
1777+
# https://github.com/bazelbuild/bazel/issues/15088.
17771778
local package="${FUNCNAME[0]}"
17781779
mkdir -p "${package}"
17791780

@@ -1785,12 +1786,9 @@ cc_test(
17851786
EOF
17861787
touch "${package}"/test.cc
17871788

1788-
# FIXME: cc_test still unconditionally depends on the LCOV merger binary through
1789-
# @remote_coverage_tools//:coverage_output_generator, which is also unnecessary:
1790-
# https://github.com/bazelbuild/bazel/issues/15088
1791-
out=$(bazel cquery "somepath(//${package}:test,@remote_coverage_tools//:lcov_merger)")
1789+
out=$(bazel cquery "somepath(//${package}:test,@remote_coverage_tools//:all)")
17921790
if [[ -n "$out" ]]; then
1793-
fail "Expected no dependency on lcov_merger, but got: $out"
1791+
fail "Expected no dependency on remote coverage tools, but got: $out"
17941792
fi
17951793
}
17961794

0 commit comments

Comments
 (0)