Merge the baseline coverage code path into the coverage codepath. We're currently doing too much work for baseline coverage - every rule creates an action for its entire transitive closure; these actions are added to the output group for baseline coverage, but not transitively accumulated. It would be better for every rule to create an action for local baseline coverage, and to aggregate the baseline coverage artifacts down the dependency tree. By merging the code paths, the InstrumentedFilesCollector can perform the aggregation, because it can distinguish local and transitive files; I'm planning to implement that in a subsequent change. -- MOS_MIGRATED_REVID=101914334
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java index 5380cf7..fea33ae 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
@@ -68,6 +68,12 @@ } @Override + public NestedSet<Artifact> getBaselineCoverageArtifacts() { + return getProvider(InstrumentedFilesProvider.class, InstrumentedFilesProviderImpl.EMPTY) + .getBaselineCoverageArtifacts(); + } + + @Override public Map<String, String> getExtraEnv() { return ImmutableMap.of(); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index 1dde2a4..1b202340 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -44,7 +44,6 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; -import com.google.devtools.build.lib.rules.test.BaselineCoverageAction; import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider; import com.google.devtools.build.lib.syntax.Label; import com.google.devtools.build.lib.util.FileType; @@ -301,7 +300,7 @@ RuleConfiguredTargetBuilder ruleBuilder = new RuleConfiguredTargetBuilder(ruleContext); addTransitiveInfoProviders( ruleContext, cppConfiguration, common, ruleBuilder, filesToBuild, ccCompilationOutputs, - cppCompilationContext, linkingOutputs, dwoArtifacts, transitiveLipoInfo); + cppCompilationContext, linkingOutputs, dwoArtifacts, transitiveLipoInfo, fake); Map<Artifact, IncludeScannable> scannableMap = new LinkedHashMap<>(); if (cppConfiguration.isLipoContextCollector()) { @@ -325,8 +324,6 @@ cppCompilationContext, ImmutableMap.copyOf(scannableMap))) .addProvider(CppLinkAction.Context.class, linkContext) .addSkylarkTransitiveInfo(CcSkylarkApiProvider.NAME, new CcSkylarkApiProvider()) - .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, - createBaselineCoverageArtifacts(ruleContext, common, ccCompilationOutputs, fake)) .build(); } @@ -569,19 +566,6 @@ return builder.build(); } - private static NestedSet<Artifact> createBaselineCoverageArtifacts( - RuleContext context, CcCommon common, CcCompilationOutputs compilationOutputs, - boolean fake) { - if (!TargetUtils.isTestRule(context.getRule()) && !fake) { - Iterable<Artifact> objectFiles = compilationOutputs.getObjectFiles( - CppHelper.usePic(context, !isLinkShared(context))); - return BaselineCoverageAction.getBaselineCoverageArtifacts(context, - common.getInstrumentedFilesProvider(objectFiles).getInstrumentedFiles()); - } else { - return NestedSetBuilder.emptySet(Order.STABLE_ORDER); - } - } - private static void addTransitiveInfoProviders( RuleContext ruleContext, CppConfiguration cppConfiguration, @@ -592,10 +576,14 @@ CppCompilationContext cppCompilationContext, CcLinkingOutputs linkingOutputs, DwoArtifactsCollector dwoArtifacts, - TransitiveLipoInfoProvider transitiveLipoInfo) { + TransitiveLipoInfoProvider transitiveLipoInfo, + boolean fake) { List<Artifact> instrumentedObjectFiles = new ArrayList<>(); instrumentedObjectFiles.addAll(ccCompilationOutputs.getObjectFiles(false)); instrumentedObjectFiles.addAll(ccCompilationOutputs.getObjectFiles(true)); + InstrumentedFilesProvider instrumentedFilesProvider = common.getInstrumentedFilesProvider( + instrumentedObjectFiles, !TargetUtils.isTestRule(ruleContext.getRule()) && !fake); + builder .setFilesToBuild(filesToBuild) .add(CppCompilationContext.class, cppCompilationContext) @@ -605,8 +593,7 @@ ruleContext, linkingOutputs.getExecutionDynamicLibraries()))) .add(CcNativeLibraryProvider.class, new CcNativeLibraryProvider( collectTransitiveCcNativeLibraries(ruleContext, linkingOutputs.getDynamicLibraries()))) - .add(InstrumentedFilesProvider.class, common.getInstrumentedFilesProvider( - instrumentedObjectFiles)) + .add(InstrumentedFilesProvider.class, instrumentedFilesProvider) .add(CppDebugFileProvider.class, new CppDebugFileProvider( dwoArtifacts.getDwoArtifacts(), dwoArtifacts.getPicDwoArtifacts())) .addOutputGroup(OutputGroupProvider.TEMP_FILES, @@ -614,7 +601,9 @@ .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, common.getFilesToCompile(ccCompilationOutputs)) .addOutputGroup(OutputGroupProvider.COMPILATION_PREREQUISITES, - CcCommon.collectCompilationPrerequisites(ruleContext, cppCompilationContext)); + CcCommon.collectCompilationPrerequisites(ruleContext, cppCompilationContext)) + .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, + instrumentedFilesProvider.getBaselineCoverageArtifacts()); } private static NestedSet<Artifact> collectExecutionDynamicLibraryArtifacts(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index 26340fb..aff5dd7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -487,11 +487,13 @@ compilationOutputs.getObjectFiles(CppHelper.usePic(ruleContext, false))); } - InstrumentedFilesProvider getInstrumentedFilesProvider(Iterable<Artifact> files) { + InstrumentedFilesProvider getInstrumentedFilesProvider(Iterable<Artifact> files, + boolean withBaselineCoverage) { return cppConfiguration.isLipoContextCollector() ? InstrumentedFilesProviderImpl.EMPTY : InstrumentedFilesCollector.collect( - ruleContext, CppRuleClasses.INSTRUMENTATION_SPEC, CC_METADATA_COLLECTOR, files); + ruleContext, CppRuleClasses.INSTRUMENTATION_SPEC, CC_METADATA_COLLECTOR, files, + withBaselineCoverage); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java index cc5a02a..b80fe17 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -37,7 +37,6 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; -import com.google.devtools.build.lib.rules.test.BaselineCoverageAction; import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider; import com.google.devtools.build.lib.syntax.Label; import com.google.devtools.build.lib.util.FileType; @@ -258,7 +257,7 @@ instrumentedObjectFiles.addAll(info.getCcCompilationOutputs().getObjectFiles(false)); instrumentedObjectFiles.addAll(info.getCcCompilationOutputs().getObjectFiles(true)); InstrumentedFilesProvider instrumentedFilesProvider = - common.getInstrumentedFilesProvider(instrumentedObjectFiles); + common.getInstrumentedFilesProvider(instrumentedObjectFiles, /*withBaselineCoverage=*/true); targetBuilder .setFilesToBuild(filesToBuild) .addProviders(info.getProviders()) @@ -269,9 +268,8 @@ // Remove this? .add(CppRunfilesProvider.class, new CppRunfilesProvider(staticRunfiles, sharedRunfiles)) .addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, artifactsToForce) - .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, BaselineCoverageAction - .getBaselineCoverageArtifacts(ruleContext, - instrumentedFilesProvider.getInstrumentedFiles())); + .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, + instrumentedFilesProvider.getBaselineCoverageArtifacts()); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java index d786c37..e17ba06 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
@@ -43,7 +43,6 @@ import com.google.devtools.build.lib.rules.cpp.LinkerInput; import com.google.devtools.build.lib.rules.java.DirectDependencyProvider.Dependency; import com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType; -import com.google.devtools.build.lib.rules.test.BaselineCoverageAction; import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector; import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector.LocalMetadataCollector; import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider; @@ -519,20 +518,16 @@ public void addTransitiveInfoProviders(RuleConfiguredTargetBuilder builder, NestedSet<Artifact> filesToBuild, @Nullable Artifact classJar) { - InstrumentedFilesProvider instrumentedFilesProvider = - InstrumentedFilesCollector.collect(ruleContext, semantics.getCoverageInstrumentationSpec(), - JAVA_METADATA_COLLECTOR, filesToBuild); + InstrumentedFilesProvider instrumentedFilesProvider = InstrumentedFilesCollector.collect( + ruleContext, semantics.getCoverageInstrumentationSpec(), JAVA_METADATA_COLLECTOR, + filesToBuild, /*withBaselineCoverage*/!TargetUtils.isTestRule(ruleContext.getTarget())); builder .add(InstrumentedFilesProvider.class, instrumentedFilesProvider) .add(JavaExportsProvider.class, new JavaExportsProvider(collectTransitiveExports())) - .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, getFilesToCompile(classJar)); - - if (!TargetUtils.isTestRule(ruleContext.getTarget())) { - builder.addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, - BaselineCoverageAction.getBaselineCoverageArtifacts(ruleContext, - instrumentedFilesProvider.getInstrumentedFiles())); - } + .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, getFilesToCompile(classJar)) + .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, + instrumentedFilesProvider.getBaselineCoverageArtifacts()); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java index f83c798..fc281d8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.rules.test; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.eventbus.EventBus; @@ -41,7 +42,8 @@ /** * Generates baseline (empty) coverage for the given non-test target. */ -public class BaselineCoverageAction extends AbstractFileWriteAction +@VisibleForTesting +public final class BaselineCoverageAction extends AbstractFileWriteAction implements NotifyOnActionCacheHit { private final Iterable<Artifact> instrumentedFiles; @@ -118,7 +120,7 @@ * Returns collection of baseline coverage artifacts associated with the given target. * Will always return 0 or 1 elements. */ - public static NestedSet<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext, + static NestedSet<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext, Iterable<Artifact> instrumentedFiles) { // Baseline coverage artifacts will still go into "testlogs" directory. Artifact coverageData = ruleContext.getPackageRelativeArtifact( @@ -129,5 +131,4 @@ return NestedSetBuilder.create(Order.STABLE_ORDER, coverageData); } - }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java index 73f4cc1..e636c8a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
@@ -41,10 +41,26 @@ public final class InstrumentedFilesCollector { public static InstrumentedFilesProvider collect(RuleContext ruleContext, InstrumentationSpec spec, LocalMetadataCollector localMetadataCollector, Iterable<Artifact> rootFiles) { + return collect(ruleContext, spec, localMetadataCollector, rootFiles, false); + } + + public static InstrumentedFilesProvider collect(RuleContext ruleContext, + InstrumentationSpec spec, LocalMetadataCollector localMetadataCollector, + Iterable<Artifact> rootFiles, boolean withBaselineCoverage) { InstrumentedFilesCollector collector = new InstrumentedFilesCollector(ruleContext, spec, localMetadataCollector, rootFiles); + NestedSet<Artifact> baselineCoverageArtifacts; + if (withBaselineCoverage) { + baselineCoverageArtifacts = + BaselineCoverageAction.getBaselineCoverageArtifacts(ruleContext, + collector.instrumentedFiles); + } else { + baselineCoverageArtifacts = NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER); + } return new InstrumentedFilesProviderImpl(collector.instrumentedFiles, - collector.instrumentationMetadataFiles, ImmutableMap.<String, String>of()); + collector.instrumentationMetadataFiles, + baselineCoverageArtifacts, + ImmutableMap.<String, String>of()); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java index 166c7e2..2d06204 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
@@ -35,6 +35,8 @@ */ NestedSet<Artifact> getInstrumentationMetadataFiles(); + NestedSet<Artifact> getBaselineCoverageArtifacts(); + /** * Returns environment variables which should be set for coverage to function. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java index 90605e6..b25b632 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
@@ -28,19 +28,29 @@ public static final InstrumentedFilesProvider EMPTY = new InstrumentedFilesProviderImpl( NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER), NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER), + NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER), ImmutableMap.<String, String>of()); private final NestedSet<Artifact> instrumentedFiles; private final NestedSet<Artifact> instrumentationMetadataFiles; + private final NestedSet<Artifact> baselineCoverageArtifacts; private final ImmutableMap<String, String> extraEnv; public InstrumentedFilesProviderImpl(NestedSet<Artifact> instrumentedFiles, - NestedSet<Artifact> instrumentationMetadataFiles, Map<String, String> extraEnv) { + NestedSet<Artifact> instrumentationMetadataFiles, + NestedSet<Artifact> baselineCoverageArtifacts, Map<String, String> extraEnv) { this.instrumentedFiles = instrumentedFiles; this.instrumentationMetadataFiles = instrumentationMetadataFiles; + this.baselineCoverageArtifacts = baselineCoverageArtifacts; this.extraEnv = ImmutableMap.copyOf(extraEnv); } + public InstrumentedFilesProviderImpl(NestedSet<Artifact> instrumentedFiles, + NestedSet<Artifact> instrumentationMetadataFiles, Map<String, String> extraEnv) { + this(instrumentedFiles, instrumentationMetadataFiles, + NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER), extraEnv); + } + @Override public NestedSet<Artifact> getInstrumentedFiles() { return instrumentedFiles; @@ -52,6 +62,11 @@ } @Override + public NestedSet<Artifact> getBaselineCoverageArtifacts() { + return baselineCoverageArtifacts; + } + + @Override public Map<String, String> getExtraEnv() { return extraEnv; }