Skip to content

Commit

Permalink
Forward InstrumentedFilesProvider from all dependencies by default (c…
Browse files Browse the repository at this point in the history 
…onditioned 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
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed May 29, 2020
1 parent 8188ccf commit 21179b2
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 46 deletions.
10 changes: 9 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -149,6 +150,14 @@ public ConfiguredTarget build() throws ActionConflictException {

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())) {
Expand Down Expand Up @@ -219,7 +228,6 @@ public ConfiguredTarget build() throws ActionConflictException {
BuildSettingProvider.class,
new BuildSettingProvider(buildSetting, defaultValue, ruleContext.getLabel()));
}

TransitiveInfoProviderMap providers = providersBuilder.build();

if (ruleContext.getRule().isAnalysisTest()) {
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,10 @@ public boolean isCodeCoverageEnabled() {
return options.collectCodeCoverage;
}

public boolean experimentalForwardInstrumentedFilesInfoByDefault() {
return options.experimentalForwardInstrumentedFilesInfoByDefault;
}

public RunUnder getRunUnder() {
return options.runUnder;
}
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,16 @@ public String getTypeDescription() {
+ " not be specified directly - 'bazel coverage' command should be used instead.")
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",
Expand Down
166 changes: 121 additions & 45 deletions src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -56,7 +59,20 @@ public static InstrumentedFilesInfo forward(
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(
Expand All @@ -66,7 +82,8 @@ public static InstrumentedFilesInfo collectTransitive(
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(
Expand Down Expand Up @@ -94,7 +111,8 @@ public static InstrumentedFilesInfo collect(
spec,
localMetadataCollector,
rootFiles,
/* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
/* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
Order.STABLE_ORDER));
}

public static InstrumentedFilesInfo collect(
Expand Down Expand Up @@ -137,7 +155,8 @@ public static InstrumentedFilesInfo collect(
coverageSupportFiles,
coverageEnvironment,
withBaselineCoverage,
/* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
/* reportedToActualSources= */ NestedSetBuilder.<Pair<String, String>>emptySet(
Order.STABLE_ORDER));
}

public static InstrumentedFilesInfo collect(
Expand All @@ -156,30 +175,14 @@ public static InstrumentedFilesInfo collect(
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.
Expand All @@ -188,7 +191,7 @@ public static InstrumentedFilesInfo collect(
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;
}
Expand All @@ -201,34 +204,20 @@ public static InstrumentedFilesInfo collect(
}
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();
}

/**
Expand Down Expand Up @@ -352,12 +341,86 @@ protected void addOutputs(NestedSetBuilder<Artifact> metadataFilesBuilder,
}
}

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) {
Expand All @@ -368,4 +431,17 @@ private static Iterable<TransitiveInfoCollection> getAllPrerequisites(
}
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;
}
}
57 changes: 57 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,63 @@ public void testTransitiveInfoProviders() throws Exception {
.containsExactly("a.txt");
}

@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");
Expand Down

0 comments on commit 21179b2

Please sign in to comment.