Do not delay TargetCompleteEvents with coverage

Baseline coverage artifacts are now requested in `CompletionFunction` to ensure that they are built before the `TargetCompleteEvent` is generated. This makes it unnecessary to delay sending these events until after a Skymeld build has had the chance to request all coverage artifacts directly, which could only be done after the analysis & execution phase, thus delaying events until the end of the build.

Fixes bazelbuild#21475

Closes bazelbuild#22238.

PiperOrigin-RevId: 631414420
Change-Id: Idc77b6f5c8b5b775e6c69e35c5563f63b3bf974f

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

@@ -338,7 +338,6 @@ java_library(
  "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
  "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
  "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
- "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event",
  "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
  "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
  "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",

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

@@ -438,7 +438,6 @@ java_library(
  "//src/main/java/com/google/devtools/build/lib/vfs",
  "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
  "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
- "//src/main/java/com/google/devtools/build/skyframe",
  "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
  "//src/main/java/com/google/devtools/common/options",
  "//src/main/java/net/starlark/java/annot",
@@ -2700,16 +2699,6 @@ java_library(
  ],
 )
 
-java_library(
- name = "test/coverage_action_finished_event",
- srcs = ["test/CoverageActionFinishedEvent.java"],
- deps = [
- "//src/main/java/com/google/devtools/build/lib/buildeventstream",
- "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
- "//third_party:guava",
- ],
-)
-
 java_library(
  name = "test/coverage_artifacts_known_event",
  srcs = ["test/CoverageArtifactsKnownEvent.java"],

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

@@ -839,32 +839,39 @@ private ImmutableSet<Artifact> memoizedGetCoverageArtifactsHelper(
  EventBus eventBus,
  TargetPatternPhaseValue loadingResult)
  throws InterruptedException {
- if (memoizedCoverageArtifacts != null) {
- return memoizedCoverageArtifacts;
+ if (memoizedCoverageArtifacts == null) {
+ memoizedCoverageArtifacts =
+ constructCoverageArtifacts(
+ configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult);
  }
- ImmutableSet.Builder<Artifact> resultBuilder = ImmutableSet.builder();
- // Coverage
- NestedSet<Artifact> baselineCoverageArtifacts = getBaselineCoverageArtifacts(configuredTargets);
- resultBuilder.addAll(baselineCoverageArtifacts.toList());
+ return memoizedCoverageArtifacts;
+ }
 
- if (coverageReportActionFactory != null) {
- CoverageReportActionsWrapper actionsWrapper =
- coverageReportActionFactory.createCoverageReportActionsWrapper(
- eventHandler,
- eventBus,
- directories,
- allTargetsToTest,
- baselineCoverageArtifacts,
- getArtifactFactory(),
- skyframeExecutor.getActionKeyContext(),
- CoverageReportValue.COVERAGE_REPORT_KEY,
- loadingResult.getWorkspaceName());
- if (actionsWrapper != null) {
- skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions());
- actionsWrapper.getCoverageOutputs().forEach(resultBuilder::add);
- }
+ private ImmutableSet<Artifact> constructCoverageArtifacts(
+ Set<ConfiguredTarget> configuredTargets,
+ Set<ConfiguredTarget> allTargetsToTest,
+ EventHandler eventHandler,
+ EventBus eventBus,
+ TargetPatternPhaseValue loadingResult)
+ throws InterruptedException {
+ if (coverageReportActionFactory == null) {
+ return ImmutableSet.of();
  }
- memoizedCoverageArtifacts = resultBuilder.build();
- return memoizedCoverageArtifacts;
+ CoverageReportActionsWrapper actionsWrapper =
+ coverageReportActionFactory.createCoverageReportActionsWrapper(
+ eventHandler,
+ eventBus,
+ directories,
+ allTargetsToTest,
+ getBaselineCoverageArtifacts(configuredTargets),
+ getArtifactFactory(),
+ skyframeExecutor.getActionKeyContext(),
+ CoverageReportValue.COVERAGE_REPORT_KEY,
+ loadingResult.getWorkspaceName());
+ if (actionsWrapper == null) {
+ return ImmutableSet.of();
+ }
+ skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions());
+ return ImmutableSet.copyOf(actionsWrapper.getCoverageOutputs());
  }
 }

src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java

@@ -174,7 +174,6 @@ private TargetCompleteEvent(
  instrumentedFilesProvider.getBaselineCoverageArtifacts();
  if (!baselineCoverageArtifacts.isEmpty()) {
  this.baselineCoverageArtifacts = baselineCoverageArtifacts;
- postedAfterBuilder.add(BuildEventIdUtil.coverageActionsFinished());
  } else {
  this.baselineCoverageArtifacts = null;
  }

src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java

@@ -162,12 +162,6 @@ public static BuildEventId targetConfigured(Label label) {
  return BuildEventId.newBuilder().setTargetConfigured(configuredId).build();
  }
 
- public static BuildEventId coverageActionsFinished() {
- return BuildEventId.newBuilder()
- .setCoverageActionsFinished(BuildEventId.CoverageActionsFinishedId.getDefaultInstance())
- .build();
- }
-
  public static BuildEventId aspectConfigured(Label label, String aspect) {
  BuildEventId.TargetConfiguredId configuredId =
  BuildEventId.TargetConfiguredId.newBuilder()

src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto

@@ -248,12 +248,11 @@ message BuildEventId {
  // Identifier of an event providing convenience symlinks information.
  message ConvenienceSymlinksIdentifiedId {}
 
- // Identifier of an event signalling that the coverage actions are finished.
- message CoverageActionsFinishedId {}
-
  // Identifier of an event providing the ExecRequest of a run command.
  message ExecRequestId {}
 
+ reserved 27;
+
  oneof id {
  UnknownBuildEventId unknown = 1;
  ProgressId progress = 2;
@@ -282,7 +281,6 @@ message BuildEventId {
  WorkspaceConfigId workspace = 23;
  BuildMetadataId build_metadata = 24;
  ConvenienceSymlinksIdentifiedId convenience_symlinks_identified = 25;
- CoverageActionsFinishedId coverage_actions_finished = 27;
  ExecRequestId exec_request = 28;
  }
 }

src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java

@@ -34,7 +34,6 @@
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
-import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent;
 import com.google.devtools.build.lib.analysis.test.TestProvider;
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.buildtool.buildevent.ExecutionProgressReceiverAvailableEvent;
@@ -127,9 +126,6 @@ public void buildArtifacts(
  skyframeExecutor
  .getEventBus()
  .post(new ExecutionProgressReceiverAvailableEvent(executionProgressReceiver));
- // When not in Skymeld mode, TargetCompleteEvents don't need to be held back.
- // See {@link CoverageActionFinishedEvent}.
- skyframeExecutor.getEventBus().post(new CoverageActionFinishedEvent());
 
  List<DetailedExitCode> detailedExitCodes = new ArrayList<>();
  EvaluationResult<?> result;

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -262,8 +262,8 @@ java_library(
  "//src/main/java/com/google/devtools/build/lib/analysis:target_configured_event",
  "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
  "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_propagation_exception",
- "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event",
  "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
+ "//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info",
  "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection",
  "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context",
  "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.SuccessfulArtifactFilter;
+import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.causes.Cause;
 import com.google.devtools.build.lib.causes.LabelCause;
@@ -147,8 +148,20 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  ValueT value = valueAndArtifactsToBuild.first;
  ArtifactsToBuild artifactsToBuild = valueAndArtifactsToBuild.second;
 
+ // Ensure that coverage artifacts are built before a target is considered completed.
  ImmutableList<Artifact> allArtifacts = artifactsToBuild.getAllArtifacts().toList();
- SkyframeLookupResult inputDeps = env.getValuesAndExceptions(Artifact.keys(allArtifacts));
+ InstrumentedFilesInfo instrumentedFilesInfo =
+ value.getConfiguredObject().get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+ Iterable<SkyKey> keysToRequest;
+ if (value.getConfiguredObject() instanceof ConfiguredTarget && instrumentedFilesInfo != null) {
+ keysToRequest =
+ Iterables.concat(
+ Artifact.keys(allArtifacts),
+ Artifact.keys(instrumentedFilesInfo.getBaselineCoverageArtifacts().toList()));
+ } else {
+ keysToRequest = Artifact.keys(allArtifacts);
+ }
+ SkyframeLookupResult inputDeps = env.getValuesAndExceptions(keysToRequest);
 
  boolean allArtifactsAreImportant = artifactsToBuild.areAllOutputGroupsImportant();
 

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java

@@ -68,7 +68,6 @@
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException;
-import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent;
 import com.google.devtools.build.lib.analysis.test.CoverageArtifactsKnownEvent;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.bugreport.BugReporter;
@@ -726,18 +725,15 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
  }
 
  // Coverage report generation should only be requested after all tests have executed.
- // We could generate baseline coverage artifacts earlier; it is only the timing of the
- // combined report that matters.
  // When --nokeep_going and there's an earlier error, we should skip this and fail fast.
  if ((!mainEvaluationResult.hasError() && !hasExclusiveTestsError) || keepGoing) {
- ImmutableSet<Artifact> coverageArtifacts =
- coverageReportActionsWrapperSupplier.getCoverageArtifacts(
+ ImmutableSet<Artifact> coverageReportArtifacts =
+ coverageReportActionsWrapperSupplier.getCoverageReportArtifacts(
  buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
- eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts));
+ eventBus.post(CoverageArtifactsKnownEvent.create(coverageReportArtifacts));
  additionalArtifactsResult =
  skyframeExecutor.evaluateSkyKeys(
- eventHandler, Artifact.keys(coverageArtifacts), keepGoing);
- eventBus.post(new CoverageActionFinishedEvent());
+ eventHandler, Artifact.keys(coverageReportArtifacts), keepGoing);
  if (additionalArtifactsResult.hasError()) {
  detailedExitCodes.add(
  SkyframeErrorProcessor.processErrors(
@@ -1449,7 +1445,7 @@ public void reset() {
  /** Provides the list of coverage artifacts to be built. */
  @FunctionalInterface
  public interface CoverageReportActionsWrapperSupplier {
- ImmutableSet<Artifact> getCoverageArtifacts(
+ ImmutableSet<Artifact> getCoverageReportArtifacts(
  Set<ConfiguredTarget> configuredTargets, Set<ConfiguredTarget> allTargetsToTest)
  throws InterruptedException;
  }