Skip to content

Commit

Permalink
Only fetch @remote_coverage_tools when collecting coverage (#17512)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
3 people authored Feb 19, 2023
1 parent a0fa77c commit 1a438b4
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 34 deletions.
21 changes: 20 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -75,4 +91,12 @@ public Label outputGenerator() {
}
return coverageOptions.coverageOutputGenerator;
}

@Nullable
public Label reportGenerator() {
if (coverageOptions == null) {
return null;
}
return coverageOptions.coverageReportGenerator;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 5 additions & 7 deletions src/test/shell/bazel/cc_integration_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"

Expand All @@ -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
}

Expand Down

0 comments on commit 1a438b4

Please sign in to comment.