Forward InstrumentedFilesProvider from all dependencies by default (conditioned on flag)
Currently, the entire dependency chain between a test and whatever rule compiles the source files must provide InstrumentedFilesInfo. Getting coverage correct for a given language requires not only rules that handle source files in that language to be correctly configured, but all rules in the dependency chain between the test and the source files must explicitly support coverage collection.
Often, a rule's immediate dependencies are all one of:
* Source files (which don't provide InstrumentedFilesProvider)
* Rule targets which might provide either code or binaries which might be executed at runtime
In those cases, forwarding InstrumentedFilesProvider from the dependencies is the correct behavior. Tool dependencies (ones in host/exec config) can be excluded, since code/binaries provided by those dependencies are not executed at runtime.
If InstrumentedFilesProvider is forwarded from a dependency incorrectly and something in that dependency's transitive dependencies is matched by --instrumentation_filter, the downside is that the user may end up with correct runtime coverage data for an overly-large list of source files. The extraneous entries will note there is no coverage data for that file. That seems possibly better than the downside of the current default, that coverage data which could be correctly gathered is potentially omitted.
The new behavior is enabled with the flag --experimental_forward_instrumented_files_info_by_default.
RELNOTES: Add experiment flag to forward InstrumentedFilesInfo from non-tool deps by default.
PiperOrigin-RevId: 313804469
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 8d0ed65..bac52e5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.analysis.test.AnalysisTestActionBuilder;
import com.google.devtools.build.lib.analysis.test.AnalysisTestResultInfo;
import com.google.devtools.build.lib.analysis.test.ExecutionInfo;
+import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
import com.google.devtools.build.lib.analysis.test.TestActionBuilder;
import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo;
@@ -149,6 +150,14 @@
collectTransitiveValidationOutputGroups();
+ // Add a default provider that forwards InstrumentedFilesInfo from dependencies, even if this
+ // rule doesn't configure InstrumentedFilesInfo. This needs to be done for non-test rules
+ // as well, but should be done before initializeTestProvider, which uses that.
+ if (ruleContext.getConfiguration().isCodeCoverageEnabled()
+ && ruleContext.getConfiguration().experimentalForwardInstrumentedFilesInfoByDefault()
+ && !providersBuilder.contains(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey())) {
+ addNativeDeclaredProvider(InstrumentedFilesCollector.forwardAll(ruleContext));
+ }
// Create test action and artifacts if target was successfully initialized
// and is a test.
if (TargetUtils.isTestRule(ruleContext.getTarget())) {
@@ -219,7 +228,6 @@
BuildSettingProvider.class,
new BuildSettingProvider(buildSetting, defaultValue, ruleContext.getLabel()));
}
-
TransitiveInfoProviderMap providers = providersBuilder.build();
if (ruleContext.getRule().isAnalysisTest()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 5f2b761..b9c1746 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -657,6 +657,10 @@
return options.collectCodeCoverage;
}
+ public boolean experimentalForwardInstrumentedFilesInfoByDefault() {
+ return options.experimentalForwardInstrumentedFilesInfoByDefault;
+ }
+
public RunUnder getRunUnder() {
return options.runUnder;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
index 7d04a83..785ef1c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -377,6 +377,16 @@
public boolean collectCodeCoverage;
@Option(
+ name = "experimental_forward_instrumented_files_info_by_default",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+ effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+ help =
+ "If specified, rules that don't configure InstrumentedFilesInfo will still forward the "
+ + "contents of InstrumentedFilesInfo from transitive dependencies.")
+ public boolean experimentalForwardInstrumentedFilesInfoByDefault;
+
+ @Option(
name = "build_runfile_manifests",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
index 8d09117..48375e1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
@@ -28,6 +28,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
@@ -42,6 +43,8 @@
*/
public final class InstrumentedFilesCollector {
+ private InstrumentedFilesCollector() {}
+
/**
* Forwards any instrumented files from the given target's dependencies (as defined in {@code
* dependencyAttributes}) for further export. No files from this target are considered
@@ -56,7 +59,20 @@
new InstrumentationSpec(FileTypeSet.NO_FILE).withDependencyAttributes(dependencyAttributes),
/* localMetadataCollector= */ null,
/* rootFiles= */ null,
- /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
+ /* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
+ Order.STABLE_ORDER));
+ }
+
+ public static InstrumentedFilesInfo forwardAll(RuleContext ruleContext) {
+ if (!ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+ return InstrumentedFilesInfo.EMPTY;
+ }
+ InstrumentedFilesInfoBuilder instrumentedFilesInfoBuilder =
+ new InstrumentedFilesInfoBuilder(ruleContext);
+ for (TransitiveInfoCollection dep : getAllNonToolPrerequisites(ruleContext)) {
+ instrumentedFilesInfoBuilder.addFromDependency(dep);
+ }
+ return instrumentedFilesInfoBuilder.build();
}
public static InstrumentedFilesInfo collectTransitive(
@@ -66,7 +82,8 @@
spec,
NO_METADATA_COLLECTOR,
ImmutableList.of(),
- /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
+ /* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
+ Order.STABLE_ORDER));
}
public static InstrumentedFilesInfo collectTransitive(
@@ -94,7 +111,8 @@
spec,
localMetadataCollector,
rootFiles,
- /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
+ /* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
+ Order.STABLE_ORDER));
}
public static InstrumentedFilesInfo collect(
@@ -137,7 +155,8 @@
coverageSupportFiles,
coverageEnvironment,
withBaselineCoverage,
- /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
+ /* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
+ Order.STABLE_ORDER));
}
public static InstrumentedFilesInfo collect(
@@ -156,30 +175,14 @@
return InstrumentedFilesInfo.EMPTY;
}
- NestedSetBuilder<Artifact> instrumentedFilesBuilder = NestedSetBuilder.stableOrder();
- NestedSetBuilder<Artifact> metadataFilesBuilder = NestedSetBuilder.stableOrder();
- NestedSetBuilder<Artifact> baselineCoverageInstrumentedFilesBuilder =
- NestedSetBuilder.stableOrder();
- NestedSetBuilder<Artifact> coverageSupportFilesBuilder =
- NestedSetBuilder.<Artifact>stableOrder()
- .addTransitive(coverageSupportFiles);
- NestedSetBuilder<Pair<String, String>> coverageEnvironmentBuilder =
- NestedSetBuilder.<Pair<String, String>>compileOrder()
- .addTransitive(coverageEnvironment);
-
+ InstrumentedFilesInfoBuilder instrumentedFilesInfoBuilder =
+ new InstrumentedFilesInfoBuilder(
+ ruleContext, coverageSupportFiles, coverageEnvironment, reportedToActualSources);
// Transitive instrumentation data.
for (TransitiveInfoCollection dep :
- getAllPrerequisites(ruleContext, spec.dependencyAttributes)) {
- InstrumentedFilesInfo provider = dep.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
- if (provider != null) {
- instrumentedFilesBuilder.addTransitive(provider.getInstrumentedFiles());
- metadataFilesBuilder.addTransitive(provider.getInstrumentationMetadataFiles());
- baselineCoverageInstrumentedFilesBuilder.addTransitive(
- provider.getBaselineCoverageInstrumentedFiles());
- coverageSupportFilesBuilder.addTransitive(provider.getCoverageSupportFiles());
- coverageEnvironmentBuilder.addTransitive(provider.getCoverageEnvironment());
- }
+ getPrerequisitesForAttributes(ruleContext, spec.dependencyAttributes)) {
+ instrumentedFilesInfoBuilder.addFromDependency(dep);
}
// Local sources.
@@ -188,7 +191,7 @@
ruleContext.getConfiguration(), ruleContext.getLabel(), ruleContext.isTestTarget())) {
NestedSetBuilder<Artifact> localSourcesBuilder = NestedSetBuilder.stableOrder();
for (TransitiveInfoCollection dep :
- getAllPrerequisites(ruleContext, spec.sourceAttributes)) {
+ getPrerequisitesForAttributes(ruleContext, spec.sourceAttributes)) {
if (!spec.splitLists && dep.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR) != null) {
continue;
}
@@ -201,34 +204,20 @@
}
localSources = localSourcesBuilder.build();
}
- instrumentedFilesBuilder.addTransitive(localSources);
+ instrumentedFilesInfoBuilder.addLocalSources(localSources);
if (withBaselineCoverage) {
// Also add the local sources to the baseline coverage instrumented sources, if the current
// rule supports baseline coverage.
// TODO(ulfjack): Generate a local baseline coverage action, and then merge at the leaves.
- baselineCoverageInstrumentedFilesBuilder.addTransitive(localSources);
+ instrumentedFilesInfoBuilder.addBaselineCoverageSources(localSources);
}
// Local metadata files.
if (localMetadataCollector != null) {
- localMetadataCollector.collectMetadataArtifacts(rootFiles,
- ruleContext.getAnalysisEnvironment(), metadataFilesBuilder);
+ instrumentedFilesInfoBuilder.collectLocalMetadata(localMetadataCollector, rootFiles);
}
- // Baseline coverage actions.
- NestedSet<Artifact> baselineCoverageFiles = baselineCoverageInstrumentedFilesBuilder.build();
-
- // Create one baseline coverage action per target, but for the transitive closure of files.
- NestedSet<Artifact> baselineCoverageArtifacts =
- BaselineCoverageAction.create(ruleContext, baselineCoverageFiles);
- return new InstrumentedFilesInfo(
- instrumentedFilesBuilder.build(),
- metadataFilesBuilder.build(),
- baselineCoverageFiles,
- baselineCoverageArtifacts,
- coverageSupportFilesBuilder.build(),
- coverageEnvironmentBuilder.build(),
- reportedToActualSources);
+ return instrumentedFilesInfoBuilder.build();
}
/**
@@ -352,12 +341,86 @@
}
}
+ private static class InstrumentedFilesInfoBuilder {
+
+ RuleContext ruleContext;
+ NestedSetBuilder<Artifact> instrumentedFilesBuilder;
+ NestedSetBuilder<Artifact> metadataFilesBuilder;
+ NestedSetBuilder<Artifact> baselineCoverageInstrumentedFilesBuilder;
+ NestedSetBuilder<Artifact> coverageSupportFilesBuilder;
+ NestedSetBuilder<Pair<String, String>> coverageEnvironmentBuilder;
+ NestedSet<Pair<String, String>> reportedToActualSources;
+
+ InstrumentedFilesInfoBuilder(
+ RuleContext ruleContext,
+ NestedSet<Artifact> coverageSupportFiles,
+ NestedSet<Pair<String, String>> coverageEnvironment,
+ NestedSet<Pair<String, String>> reportedToActualSources) {
+ this.ruleContext = ruleContext;
+ instrumentedFilesBuilder = NestedSetBuilder.stableOrder();
+ metadataFilesBuilder = NestedSetBuilder.stableOrder();
+ baselineCoverageInstrumentedFilesBuilder = NestedSetBuilder.stableOrder();
+ coverageSupportFilesBuilder =
+ NestedSetBuilder.<Artifact>stableOrder().addTransitive(coverageSupportFiles);
+ coverageEnvironmentBuilder =
+ NestedSetBuilder.<Pair<String, String>>compileOrder().addTransitive(coverageEnvironment);
+ this.reportedToActualSources = reportedToActualSources;
+ }
+
+ InstrumentedFilesInfoBuilder(RuleContext ruleContext) {
+ this(
+ ruleContext,
+ NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER),
+ NestedSetBuilder.<Pair<String, String>>emptySet(Order.STABLE_ORDER),
+ NestedSetBuilder.<Pair<String, String>>emptySet(Order.STABLE_ORDER));
+ }
+
+ void addFromDependency(TransitiveInfoCollection dep) {
+ InstrumentedFilesInfo provider = dep.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+ if (provider != null) {
+ instrumentedFilesBuilder.addTransitive(provider.getInstrumentedFiles());
+ metadataFilesBuilder.addTransitive(provider.getInstrumentationMetadataFiles());
+ baselineCoverageInstrumentedFilesBuilder.addTransitive(
+ provider.getBaselineCoverageInstrumentedFiles());
+ coverageSupportFilesBuilder.addTransitive(provider.getCoverageSupportFiles());
+ coverageEnvironmentBuilder.addTransitive(provider.getCoverageEnvironment());
+ }
+ }
+
+ void addLocalSources(NestedSet<Artifact> localSources) {
+ instrumentedFilesBuilder.addTransitive(localSources);
+ }
+
+ void addBaselineCoverageSources(NestedSet<Artifact> localSources) {
+ baselineCoverageInstrumentedFilesBuilder.addTransitive(localSources);
+ }
+
+ void collectLocalMetadata(
+ LocalMetadataCollector localMetadataCollector, Iterable<Artifact> rootFiles) {
+ localMetadataCollector.collectMetadataArtifacts(
+ rootFiles, ruleContext.getAnalysisEnvironment(), metadataFilesBuilder);
+ }
+
+ InstrumentedFilesInfo build() {
+ NestedSet<Artifact> baselineCoverageFiles = baselineCoverageInstrumentedFilesBuilder.build();
+ return new InstrumentedFilesInfo(
+ instrumentedFilesBuilder.build(),
+ metadataFilesBuilder.build(),
+ baselineCoverageFiles,
+ // Create one baseline coverage action per target, for the transitive closure of files.
+ BaselineCoverageAction.create(ruleContext, baselineCoverageFiles),
+ coverageSupportFilesBuilder.build(),
+ coverageEnvironmentBuilder.build(),
+ reportedToActualSources);
+ }
+ }
+
/**
* An explicit constant for a {@link LocalMetadataCollector} that doesn't collect anything.
*/
public static final LocalMetadataCollector NO_METADATA_COLLECTOR = null;
- private static Iterable<TransitiveInfoCollection> getAllPrerequisites(
+ private static Iterable<TransitiveInfoCollection> getPrerequisitesForAttributes(
RuleContext ruleContext, Collection<String> attributeNames) {
List<TransitiveInfoCollection> prerequisites = new ArrayList<>();
for (String attr : attributeNames) {
@@ -368,4 +431,17 @@
}
return prerequisites;
}
+
+ private static Iterable<TransitiveInfoCollection> getAllNonToolPrerequisites(
+ RuleContext ruleContext) {
+ List<TransitiveInfoCollection> prerequisites = new ArrayList<>();
+ for (Attribute attr : ruleContext.getRule().getAttributes()) {
+ if ((attr.getType() == BuildType.LABEL_LIST || attr.getType() == BuildType.LABEL)
+ && !attr.getTransitionFactory().isTool()) {
+ prerequisites.addAll(
+ ruleContext.getPrerequisites(attr.getName(), TransitionMode.DONT_CHECK));
+ }
+ }
+ return prerequisites;
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
index 7b6d2e6..ddc5997 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
@@ -859,6 +859,63 @@
}
@Test
+ public void testInstrumentedFilesForwardedFromDepsByDefaultExperimentFlag() throws Exception {
+ scratch.file(
+ "test/starlark/extension.bzl",
+ "def wrapper_impl(ctx):",
+ // This wrapper doesn't configure InstrumentedFilesInfo.
+ " return []",
+ "",
+ "wrapper = rule(implementation = wrapper_impl,",
+ " attrs = {",
+ " 'srcs': attr.label_list(allow_files = True),",
+ " 'wrapped': attr.label(mandatory = True),",
+ " 'wrapped_list': attr.label_list(),",
+ // Host deps aren't forwarded by default, since they don't provide code/binaries executed
+ // at runtime.
+ " 'tool': attr.label(cfg = 'host', executable = True, mandatory = True),",
+ " })");
+
+ scratch.file(
+ "test/starlark/BUILD",
+ "load('//test/starlark:extension.bzl', 'wrapper')",
+ "",
+ "cc_binary(name = 'tool', srcs = [':tool.cc'])",
+ "cc_binary(name = 'wrapped', srcs = [':wrapped.cc'])",
+ "cc_binary(name = 'wrapped_list', srcs = [':wrapped_list.cc'])",
+ "wrapper(",
+ " name = 'wrapper',",
+ " srcs = ['ignored.cc'],",
+ " wrapped = ':wrapped',",
+ " wrapped_list = [':wrapped_list'],",
+ " tool = ':tool',",
+ ")",
+ "cc_binary(name = 'outer', data = [':wrapper'])");
+
+ // Current behavior is that nothing gets forwarded if IntstrumentedFilesInfo is not configured.
+ // That means that source files are not collected for the coverage manifest unless the entire
+ // dependency chain between the test and the source file explicitly configures coverage.
+ // New behavior is protected by --experimental_forward_instrumented_files_info_by_default.
+ useConfiguration("--collect_code_coverage");
+ ConfiguredTarget target = getConfiguredTarget("//test/starlark:outer");
+ InstrumentedFilesInfo provider = target.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+ assertWithMessage("InstrumentedFilesInfo should be set.").that(provider).isNotNull();
+ assertThat(ActionsTestUtil.baseArtifactNames(provider.getInstrumentedFiles())).isEmpty();
+
+ // Instead, the default behavior could be to forward InstrumentedFilesInfo from all
+ // dependencies. Coverage still needs to be configured for rules that handle source files for
+ // languages which support coverage instrumentation, but not every wrapper rule in the
+ // dependency chain needs to configure that for instrumentation to be correct.
+ useConfiguration(
+ "--collect_code_coverage", "--experimental_forward_instrumented_files_info_by_default");
+ target = getConfiguredTarget("//test/starlark:outer");
+ provider = target.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+ assertWithMessage("InstrumentedFilesInfo should be set.").that(provider).isNotNull();
+ assertThat(ActionsTestUtil.baseArtifactNames(provider.getInstrumentedFiles()))
+ .containsExactly("wrapped.cc", "wrapped_list.cc");
+ }
+
+ @Test
public void testMandatoryProviderMissing() throws Exception {
scratch.file("test/skylark/BUILD");
scratch.file(
