Add the missing build status for skymeld.

joeleba · copybara-github · commit 4fca652fdf3c · 2022-07-08T01:01:50.000-07:00
Skymeld, prior to this CL, has been missing the aggregators setup for targets. Therefore the targets' statuses in the BEP were always reported as "aborted". This CL adds the aggregators when a target (including tests) is ready to be executed.

PiperOrigin-RevId: 459696157
Change-Id: I109cbdef7051cbcc73cc0b11e5364a7612c140a3
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java b/src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java
@@ -34,7 +34,9 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
+import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetPendingExecutionEvent;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.concurrent.GuardedBy;
 import java.util.Collection;
 import java.util.concurrent.ConcurrentHashMap;
@@ -79,8 +81,6 @@ public void buildStarting(BuildStartingEvent event) {
    */
   @Subscribe
   public void populateTargets(TestFilteringCompleteEvent event) {
-    int expectedCompletions = aspectCount.get() + 1; // + 1 for target itself
-    checkState(expectedCompletions > 0, "Haven't received BuildStartingEvent");
     ImmutableSet<ConfiguredTarget> testTargets =
         event.getTestTargets() != null
             ? ImmutableSet.copyOf(event.getTestTargets())
@@ -92,16 +92,48 @@ public void populateTargets(TestFilteringCompleteEvent event) {
         // we'll still get (and ignore) a TestSummary event, but that event isn't published to BEP.
         continue;
       }
-      // We want target summaries for alias targets, but note they don't receive test summaries.
-      TargetSummaryAggregator aggregator =
-          new TargetSummaryAggregator(
-              target,
-              expectedCompletions,
-              !AliasProvider.isAlias(target) && testTargets.contains(target));
-      TargetSummaryAggregator oldAggregator = aggregators.put(asKey(target), aggregator);
+      TargetSummaryAggregator oldAggregator =
+          replaceAggregatorForTarget(/*isTest=*/ testTargets.contains(target), target);
       checkState(
-          oldAggregator == null, "target: %s, values: %s %s", target, oldAggregator, aggregator);
+          oldAggregator == null,
+          "target: %s, values: %s %s",
+          target,
+          oldAggregator,
+          aggregators.get(asKey(target)));
+    }
+  }
+
+  /**
+   * Populates the aggregator for a particular top level target, including test targets.
+   *
+   * <p>Since the event is fired from within a SkyFunction, it is possible to receive duplicate
+   * events. In case of duplication, simply return without creating any new aggregator.
+   */
+  @Subscribe
+  @AllowConcurrentEvents
+  public void populateTarget(TopLevelTargetPendingExecutionEvent event) {
+    replaceAggregatorForTarget(event.isTest(), event.configuredTarget());
+  }
+
+  /**
+   * Creates a TargetSummaryAggregator for the given target and stores it in {@link aggregators}
+   *
+   * @return the existing aggregator, if any.
+   */
+  @Nullable
+  @CanIgnoreReturnValue
+  private TargetSummaryAggregator replaceAggregatorForTarget(
+      boolean isTest, ConfiguredTarget target) {
+    if (aggregators.containsKey(asKey(target))) {
+      return null;
     }
+    int expectedCompletions = aspectCount.get() + 1; // + 1 for target itself
+    checkState(expectedCompletions > 0, "Haven't received BuildStartingEvent");
+    // We want target summaries for alias targets, but note they don't receive test summaries.
+    TargetSummaryAggregator aggregator =
+        new TargetSummaryAggregator(
+            target, expectedCompletions, isTest && !AliasProvider.isAlias(target));
+    return aggregators.put(asKey(target), aggregator);
   }
 
   @Subscribe
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java
@@ -54,15 +54,15 @@
 import com.google.devtools.build.lib.skyframe.AspectCompletionValue.AspectCompletionKey;
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
 import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.AspectAnalyzedEvent;
+import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.SomeExecutionStartedEvent;
 import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TestAnalyzedEvent;
 import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetAnalyzedEvent;
-import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetExecutionStartedEvent;
+import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetPendingExecutionEvent;
 import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetSkippedEvent;
 import com.google.devtools.build.lib.util.RegexFilter;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
 import com.google.devtools.build.skyframe.SkyFunctionException;
-import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.SkyframeIterableResult;
@@ -187,6 +187,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         }
       }
 
+      env.getListener()
+          .post(
+              TopLevelTargetPendingExecutionEvent.create(
+                  configuredTarget, buildDriverKey.isTest()));
       requestConfiguredTargetExecution(
           configuredTarget,
           buildDriverKey,
@@ -296,7 +300,7 @@ private void requestConfiguredTargetExecution(
     ImmutableSet.Builder<Artifact> artifactsToBuild = ImmutableSet.builder();
     addExtraActionsIfRequested(
         configuredTarget.getProvider(ExtraActionArtifactsProvider.class), artifactsToBuild);
-    env.getListener().post(TopLevelTargetExecutionStartedEvent.create());
+    env.getListener().post(SomeExecutionStartedEvent.create());
     if (NOT_TEST.equals(buildDriverKey.getTestType())) {
       declareDependenciesAndCheckValues(
           env,
@@ -337,7 +341,7 @@ private void requestAspectExecution(
       TopLevelArtifactContext topLevelArtifactContext)
       throws InterruptedException {
 
-    env.getListener().post(TopLevelTargetExecutionStartedEvent.create());
+    env.getListener().post(SomeExecutionStartedEvent.create());
     ImmutableSet.Builder<Artifact> artifactsToBuild = ImmutableSet.builder();
     List<SkyKey> aspectCompletionKeys = new ArrayList<>();
     for (SkyValue aspectValue : topLevelAspectsValue.getTopLevelAspectsValues()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java
@@ -84,7 +84,7 @@ public ActionLookupKey getActionLookupKey() {
   }
 
   public boolean isTest() {
-    return TestType.NOT_TEST.equals(testType);
+    return !TestType.NOT_TEST.equals(testType);
   }
 
   public TestType getTestType() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelStatusEvents.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelStatusEvents.java
@@ -53,12 +53,29 @@ public static TopLevelTargetSkippedEvent create(ConfiguredTarget configuredTarge
     }
   }
 
-  /** An event that marks the start of execution of a top-level target, including tests. */
+  /**
+   * An event that marks that a top-level target won't be skipped and is pending execution,
+   * including test targets.
+   */
   @AutoValue
-  public abstract static class TopLevelTargetExecutionStartedEvent implements Postable {
+  public abstract static class TopLevelTargetPendingExecutionEvent implements Postable {
+    public abstract ConfiguredTarget configuredTarget();
+
+    public abstract boolean isTest();
+
+    public static TopLevelTargetPendingExecutionEvent create(
+        ConfiguredTarget configuredTarget, boolean isTest) {
+      return new AutoValue_TopLevelStatusEvents_TopLevelTargetPendingExecutionEvent(
+          configuredTarget, isTest);
+    }
+  }
+
+  /** An event that denotes that some execution has started in this build. */
+  @AutoValue
+  public abstract static class SomeExecutionStartedEvent implements Postable {
 
-    public static TopLevelTargetExecutionStartedEvent create() {
-      return new AutoValue_TopLevelStatusEvents_TopLevelTargetExecutionStartedEvent();
+    public static SomeExecutionStartedEvent create() {
+      return new AutoValue_TopLevelStatusEvents_SomeExecutionStartedEvent();
     }
   }
   /** An event that marks the successful build of a top-level target, including tests. */

Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ public ActionLookupKey getActionLookupKey() {`
`84`	`84`	`}`
`85`	`85`
`86`	`86`	`public boolean isTest() {`
`87`		`- return TestType.NOT_TEST.equals(testType);`
	`87`	`+ return !TestType.NOT_TEST.equals(testType);`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`public TestType getTestType() {`