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 #21475 Closes #22238. PiperOrigin-RevId: 631414420 Change-Id: Idc77b6f5c8b5b775e6c69e35c5563f63b3bf974f
bazelbuild · May 7, 2024 · fd2cc92 · fd2cc92
1 parent 2a61fe7
commit fd2cc92
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 109 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -349,7 +349,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",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -437,7 +437,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",
@@ -2741,16 +2740,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"],

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -853,32 +853,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());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/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;
       }

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java b/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()

diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/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;
   }
 }

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/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;

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -274,8 +274,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",

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -43,6 +43,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;
@@ -170,8 +171,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();
 

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -73,7 +73,6 @@
 import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkAttributeTransitionProvider;
 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;
@@ -757,18 +756,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(
@@ -1483,7 +1479,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;
   }