Move TargetCompleteEvent generation to the CompletionFunction

This change breaks without https://github.com/bazelbuild/bazel/commit/a0ea569f7df19b8284846a52854e73747f7ec005; if that change is rolled back, this
change has to be rolled back as well.

It was previously in ExecutionProgressReceiver, and directly hooked into
Skyframe. Now that we have a way to post event bus events directly from
Sky functions, we should just do that.

Also, this allows us to access artifact metdata, since the completion function
has all the necessary artifacts as dependencies, which in turn can be used to
improve the build event protocol implementation, where we want to post URLs to
the CAS, which requires knowing the checksum.

PiperOrigin-RevId: 179903333

7 files changed, 76 insertions, 83 deletions

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
index 4a6e8aaa71..c1e7805bf7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
@@ -57,7 +57,7 @@ public final class TargetCompleteEvent
         BuildEventWithConfiguration {
   private final ConfiguredTarget target;
   private final NestedSet<Cause> rootCauses;
-  private final Collection<BuildEventId> postedAfter;
+  private final ImmutableList<BuildEventId> postedAfter;
   private final Iterable<ArtifactsInOutputGroup> outputs;
   private final NestedSet<Artifact> baselineCoverageArtifacts;
   private final boolean isTest;
@@ -71,7 +71,7 @@ public final class TargetCompleteEvent
     this.rootCauses =
         (rootCauses == null) ? NestedSetBuilder.<Cause>emptySet(Order.STABLE_ORDER) : rootCauses;
 
-    ImmutableList.Builder postedAfterBuilder = ImmutableList.builder();
+    ImmutableList.Builder<BuildEventId> postedAfterBuilder = ImmutableList.builder();
     for (Cause cause : getRootCauses()) {
       postedAfterBuilder.add(BuildEventId.fromCause(cause));
     }
@@ -94,13 +94,13 @@ public final class TargetCompleteEvent
   }
 
   /** Construct a successful target completion event. */
-  public static TargetCompleteEvent createSuccessfulTarget(
+  public static TargetCompleteEvent successfulBuild(
       ConfiguredTarget ct, NestedSet<ArtifactsInOutputGroup> outputs) {
     return new TargetCompleteEvent(ct, null, outputs, false);
   }
 
   /** Construct a successful target completion event for a target that will be tested. */
-  public static TargetCompleteEvent createSuccessfulTestTarget(ConfiguredTarget ct) {
+  public static TargetCompleteEvent successfulBuildSchedulingTest(ConfiguredTarget ct) {
     return new TargetCompleteEvent(ct, null, ImmutableList.<ArtifactsInOutputGroup>of(), true);
   }
 
@@ -147,7 +147,7 @@ public final class TargetCompleteEvent
 
   @Override
   public Collection<BuildEventId> getChildrenEvents() {
-    ImmutableList.Builder childrenBuilder = ImmutableList.builder();
+    ImmutableList.Builder<BuildEventId> childrenBuilder = ImmutableList.builder();
     for (Cause cause : getRootCauses()) {
       childrenBuilder.add(BuildEventId.fromCause(cause));
     }
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
index 4283ff5418..ef4319573a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
@@ -15,21 +15,13 @@ package com.google.devtools.build.lib.buildtool;
 
 import com.google.common.base.Supplier;
 import com.google.common.collect.Sets;
-import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
 import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
-import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.skyframe.ActionExecutionInactivityWatchdog;
-import com.google.devtools.build.lib.skyframe.AspectCompletionValue;
-import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor;
 import com.google.devtools.build.lib.skyframe.TargetCompletionValue;
@@ -62,9 +54,6 @@ public final class ExecutionProgressReceiver
   private final Object activityIndicator = new Object();
   /** Number of exclusive tests. To be accounted for in progress messages. */
   private final int exclusiveTestsCount;
-  private final Set<ConfiguredTarget> testedTargets;
-  private final EventBus eventBus;
-  private final TopLevelArtifactContext topLevelArtifactContext;
 
   static {
     PROGRESS_MESSAGE_NUMBER_FORMATTER = NumberFormat.getIntegerInstance(Locale.ENGLISH);
@@ -77,15 +66,9 @@ public final class ExecutionProgressReceiver
    */
   ExecutionProgressReceiver(
       Set<ConfiguredTarget> builtTargets,
-      int exclusiveTestsCount,
-      Set<ConfiguredTarget> testedTargets,
-      TopLevelArtifactContext topLevelArtifactContext,
-      EventBus eventBus) {
+      int exclusiveTestsCount) {
     this.builtTargets = Collections.synchronizedSet(builtTargets);
     this.exclusiveTestsCount = exclusiveTestsCount;
-    this.testedTargets = testedTargets;
-    this.topLevelArtifactContext = topLevelArtifactContext;
-    this.eventBus = eventBus;
   }
 
   @Override
@@ -123,20 +106,6 @@ public final class ExecutionProgressReceiver
 
       ConfiguredTarget target = value.getConfiguredTarget();
       builtTargets.add(target);
-
-      if (testedTargets.contains(target)) {
-        postTestTargetComplete(target);
-      } else {
-        postBuildTargetComplete(target);
-      }
-    } else if (type.equals(SkyFunctions.ASPECT_COMPLETION)) {
-      AspectCompletionValue value = (AspectCompletionValue) skyValueSupplier.get();
-      if (value != null) {
-        AspectValue aspectValue = value.getAspectValue();
-        ArtifactsToBuild artifacts =
-            TopLevelArtifactHelper.getAllArtifactsToBuild(aspectValue, topLevelArtifactContext);
-        eventBus.post(AspectCompleteEvent.createSuccessful(aspectValue, artifacts));
-      }
     } else if (type.equals(SkyFunctions.ACTION_EXECUTION)) {
       // Remember all completed actions, even those in error, regardless of having been cached or
       // really executed.
@@ -235,16 +204,4 @@ public final class ExecutionProgressReceiver
       }
     };
   }
-
-  private void postTestTargetComplete(ConfiguredTarget target) {
-    eventBus.post(TargetCompleteEvent.createSuccessfulTestTarget(target));
-  }
-
-  private void postBuildTargetComplete(ConfiguredTarget target) {
-    ArtifactsToBuild artifactsToBuild =
-        TopLevelArtifactHelper.getAllArtifactsToBuild(target, topLevelArtifactContext);
-    eventBus.post(
-        TargetCompleteEvent.createSuccessfulTarget(
-            target, artifactsToBuild.getAllArtifactsByOutputGroup()));
-  }
 }
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
index 99e8841f1d..e72eda9ed0 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -114,13 +114,7 @@ public class SkyframeBuilder implements Builder {
     ExecutionProgressReceiver executionProgressReceiver =
         new ExecutionProgressReceiver(
             Preconditions.checkNotNull(builtTargets),
-            countTestActions(exclusiveTests),
-            ImmutableSet.<ConfiguredTarget>builder()
-                .addAll(parallelTests)
-                .addAll(exclusiveTests)
-                .build(),
-            topLevelArtifactContext,
-            skyframeExecutor.getEventBus());
+            countTestActions(exclusiveTests));
     skyframeExecutor
         .getEventBus()
         .post(new ExecutionProgressReceiverAvailableEvent(executionProgressReceiver));
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
index 103fd73167..f4fb386996 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
@@ -29,6 +28,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.skyframe.AspectCompletionValue.AspectCompletionKey;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey;
@@ -38,7 +38,6 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 import java.util.Map;
-import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Nullable;
 
 /**
@@ -86,7 +85,11 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
     TResult createResult(TValue value);
 
     /** Creates a failed completion value. */
-    SkyValue createFailed(TValue value, NestedSet<Cause> rootCauses);
+    ExtendedEventHandler.Postable createFailed(TValue value, NestedSet<Cause> rootCauses);
+
+    /** Creates a succeeded completion value. */
+    ExtendedEventHandler.Postable createSucceeded(
+        SkyKey skyKey, TValue value, TopLevelArtifactContext topLevelArtifactContext);
 
     /**
      * Extracts a tag given the {@link SkyKey}.
@@ -143,7 +146,8 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
     }
 
     @Override
-    public SkyValue createFailed(ConfiguredTargetValue value, NestedSet<Cause> rootCauses) {
+    public ExtendedEventHandler.Postable createFailed(
+        ConfiguredTargetValue value, NestedSet<Cause> rootCauses) {
       return TargetCompleteEvent.createFailed(value.getConfiguredTarget(), rootCauses);
     }
 
@@ -152,6 +156,22 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
       return Label.print(
           ((TargetCompletionKey) skyKey.argument()).configuredTargetKey().getLabel());
     }
+
+    @Override
+    public ExtendedEventHandler.Postable createSucceeded(
+        SkyKey skyKey,
+        ConfiguredTargetValue value,
+        TopLevelArtifactContext topLevelArtifactContext) {
+      ConfiguredTarget target = value.getConfiguredTarget();
+      if (((TargetCompletionKey) skyKey.argument()).willTest()) {
+        return TargetCompleteEvent.successfulBuildSchedulingTest(target);
+      } else {
+        ArtifactsToBuild artifactsToBuild =
+            TopLevelArtifactHelper.getAllArtifactsToBuild(target, topLevelArtifactContext);
+        return TargetCompleteEvent.successfulBuild(
+            target, artifactsToBuild.getAllArtifactsByOutputGroup());
+      }
+    }
   }
 
   private static class AspectCompletor implements Completor<AspectValue, AspectCompletionValue> {
@@ -203,7 +223,8 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
     }
 
     @Override
-    public SkyValue createFailed(AspectValue value, NestedSet<Cause> rootCauses) {
+    public ExtendedEventHandler.Postable createFailed(
+        AspectValue value, NestedSet<Cause> rootCauses) {
       return AspectCompleteEvent.createFailed(value, rootCauses);
     }
 
@@ -211,22 +232,27 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
     public String extractTag(SkyKey skyKey) {
       return Label.print(((AspectCompletionKey) skyKey.argument()).aspectKey().getLabel());
     }
+
+    @Override
+    public ExtendedEventHandler.Postable createSucceeded(
+        SkyKey skyKey, AspectValue value, TopLevelArtifactContext topLevelArtifactContext) {
+      ArtifactsToBuild artifacts =
+          TopLevelArtifactHelper.getAllArtifactsToBuild(value, topLevelArtifactContext);
+      return AspectCompleteEvent.createSuccessful(value, artifacts);
+    }
   }
 
-  public static SkyFunction targetCompletionFunction(AtomicReference<EventBus> eventBusRef) {
-    return new CompletionFunction<>(eventBusRef, new TargetCompletor());
+  public static SkyFunction targetCompletionFunction() {
+    return new CompletionFunction<>(new TargetCompletor());
   }
 
-  public static SkyFunction aspectCompletionFunction(AtomicReference<EventBus> eventBusRef) {
-    return new CompletionFunction<>(eventBusRef, new AspectCompletor());
+  public static SkyFunction aspectCompletionFunction() {
+    return new CompletionFunction<>(new AspectCompletor());
   }
 
-  private final AtomicReference<EventBus> eventBusRef;
   private final Completor<TValue, TResult> completor;
 
-  private CompletionFunction(
-      AtomicReference<EventBus> eventBusRef, Completor<TValue, TResult> completor) {
-    this.eventBusRef = eventBusRef;
+  private CompletionFunction(Completor<TValue, TResult> completor) {
     this.completor = completor;
   }
 
@@ -280,7 +306,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
 
     NestedSet<Cause> rootCauses = rootCausesBuilder.build();
     if (!rootCauses.isEmpty()) {
-      eventBusRef.get().post(completor.createFailed(value, rootCauses));
+      env.getListener().post(completor.createFailed(value, rootCauses));
       if (firstActionExecutionException != null) {
         throw new CompletionFunctionException(firstActionExecutionException);
       } else {
@@ -288,7 +314,14 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
       }
     }
 
-    return env.valuesMissing() ? null : completor.createResult(value);
+    // Only check for missing values *after* reporting errors: if there are missing files in a build
+    // with --nokeep_going, there may be missing dependencies during error bubbling, we still need
+    // to report the error.
+    if (env.valuesMissing()) {
+      return null;
+    }
+    env.getListener().post(completor.createSucceeded(skyKey, value, topLevelContext));
+    return completor.createResult(value);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 6bca56719a..736b8fd10c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -439,8 +439,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         SkyFunctions.WORKSPACE_FILE,
         new WorkspaceFileFunction(ruleClassProvider, pkgFactory, directories));
     map.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction());
-    map.put(SkyFunctions.TARGET_COMPLETION, CompletionFunction.targetCompletionFunction(eventBus));
-    map.put(SkyFunctions.ASPECT_COMPLETION, CompletionFunction.aspectCompletionFunction(eventBus));
+    map.put(SkyFunctions.TARGET_COMPLETION, CompletionFunction.targetCompletionFunction());
+    map.put(SkyFunctions.ASPECT_COMPLETION, CompletionFunction.aspectCompletionFunction());
     map.put(SkyFunctions.TEST_COMPLETION, new TestCompletionFunction());
     map.put(SkyFunctions.ARTIFACT, new ArtifactFunction());
     map.put(
@@ -1202,7 +1202,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       Set<Artifact> artifactsToBuild,
       Collection<ConfiguredTarget> targetsToBuild,
       Collection<AspectValue> aspects,
-      Collection<ConfiguredTarget> targetsToTest,
+      Set<ConfiguredTarget> targetsToTest,
       boolean exclusiveTesting,
       boolean keepGoing,
       boolean explain,
@@ -1224,7 +1224,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       progressReceiver.executionProgressReceiver = executionProgressReceiver;
       Iterable<SkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild);
       Iterable<SkyKey> targetKeys =
-          TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext);
+          TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext, targetsToTest);
       Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext);
       Iterable<SkyKey> testKeys =
           TestCompletionValue.keys(targetsToTest, topLevelArtifactContext, exclusiveTesting);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java
index c08f5a6522..b3b62e1abb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.skyframe.LegacySkyKey;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Collection;
+import java.util.Set;
 
 /**
  * The value of a TargetCompletion. Currently this just stores a ConfiguredTarget.
@@ -38,14 +39,18 @@ public class TargetCompletionValue implements SkyValue {
   }
 
   public static SkyKey key(
-      ConfiguredTargetKey configuredTargetKey, TopLevelArtifactContext topLevelArtifactContext) {
+      ConfiguredTargetKey configuredTargetKey,
+      TopLevelArtifactContext topLevelArtifactContext,
+      boolean willTest) {
     return LegacySkyKey.create(
         SkyFunctions.TARGET_COMPLETION,
-        TargetCompletionKey.create(configuredTargetKey, topLevelArtifactContext));
+        TargetCompletionKey.create(configuredTargetKey, topLevelArtifactContext, willTest));
   }
 
-  public static Iterable<SkyKey> keys(Collection<ConfiguredTarget> targets,
-      final TopLevelArtifactContext ctx) {
+  public static Iterable<SkyKey> keys(
+      Collection<ConfiguredTarget> targets,
+      final TopLevelArtifactContext ctx,
+      final Set<ConfiguredTarget> targetsToTest) {
     return Iterables.transform(
         targets,
         new Function<ConfiguredTarget, SkyKey>() {
@@ -53,7 +58,8 @@ public class TargetCompletionValue implements SkyValue {
           public SkyKey apply(ConfiguredTarget ct) {
             return LegacySkyKey.create(
                 SkyFunctions.TARGET_COMPLETION,
-                TargetCompletionKey.create(ConfiguredTargetKey.of(ct), ctx));
+                TargetCompletionKey.create(
+                    ConfiguredTargetKey.of(ct), ctx, targetsToTest.contains(ct)));
           }
         });
   }
@@ -61,13 +67,16 @@ public class TargetCompletionValue implements SkyValue {
   @AutoValue
   abstract static class TargetCompletionKey {
     public static TargetCompletionKey create(
-        ConfiguredTargetKey configuredTargetKey, TopLevelArtifactContext topLevelArtifactContext) {
+        ConfiguredTargetKey configuredTargetKey,
+        TopLevelArtifactContext topLevelArtifactContext,
+        boolean willTest) {
       return new AutoValue_TargetCompletionValue_TargetCompletionKey(
-          configuredTargetKey, topLevelArtifactContext);
+          configuredTargetKey, topLevelArtifactContext, willTest);
     }
 
     abstract ConfiguredTargetKey configuredTargetKey();
 
     public abstract TopLevelArtifactContext topLevelArtifactContext();
+    public abstract boolean willTest();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
index d476098c69..3efc7e84f6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
@@ -34,7 +34,7 @@ public final class TestCompletionFunction implements SkyFunction {
         (TestCompletionValue.TestCompletionKey) skyKey.argument();
     ConfiguredTargetKey lac = key.configuredTargetKey();
     TopLevelArtifactContext ctx = key.topLevelArtifactContext();
-    if (env.getValue(TargetCompletionValue.key(lac, ctx)) == null) {
+    if (env.getValue(TargetCompletionValue.key(lac, ctx, /*willTest=*/true)) == null) {
       return null;
     }