Fix build results for aspect builds.

The current output was pretty much completely incorrect. However since the result output was always hidden for the default value of --show_result, users simply didn't see the incorrect output (instead getting no output at all).

This CL fixes both the --show_result problem and makes the output correct.

RELNOTES: Print correct build result for builds with --aspects flag.
PiperOrigin-RevId: 191456352

5 files changed, 155 insertions, 91 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
index b8dedd1973..5ca0204cf6 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
@@ -19,6 +19,7 @@ import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
+import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.util.ExitCode;
 import java.util.Collection;
 import java.util.Collections;
@@ -42,6 +43,7 @@ public final class BuildResult {
   private Collection<ConfiguredTarget> testTargets;
   private Collection<ConfiguredTarget> successfulTargets;
   private Collection<ConfiguredTarget> skippedTargets;
+  private Collection<AspectValue> successfulAspects;
 
   public BuildResult(long startTimeMillis) {
     this.startTimeMillis = startTimeMillis;
@@ -194,20 +196,36 @@ public final class BuildResult {
     this.successfulTargets = successfulTargets;
   }
 
+  /** @see #getSuccessfulAspects */
+  void setSuccessfulAspects(Collection<AspectValue> successfulAspects) {
+    this.successfulAspects = successfulAspects;
+  }
+
   /**
-   * Returns the set of targets which successfully built.  This value
-   * is set at the end of the build, after the target patterns have been parsed
-   * and resolved and after attempting to build the targets.  If --keep_going
-   * is specified, this set may exclude targets that could not be found or
-   * successfully analyzed, or could not be built.  It may be examined after
-   * the build.  May be null if the execution phase was not attempted, as
-   * may happen if there are errors in the loading phase, for example.
+   * Returns the set of targets that were successfully built. This value is set at the end of the
+   * build, after the target patterns have been parsed and resolved and after attempting to build
+   * the targets. If --keep_going is specified, this set may exclude targets that could not be found
+   * or successfully analyzed, or could not be built. It may be examined after the build. May be
+   * null if the execution phase was not attempted, as may happen if there are errors in the loading
+   * phase, for example.
    */
   public Collection<ConfiguredTarget> getSuccessfulTargets() {
     return successfulTargets;
   }
 
   /**
+   * Returns the set of aspects that were successfully built. This value is set at the end of the
+   * build, after the target patterns have been parsed and resolved and after attempting to build
+   * the targets. If --keep_going is specified, this set may exclude targets that could not be found
+   * or successfully analyzed, or could not be built. It may be examined after the build. May be
+   * null if the execution phase was not attempted, as may happen if there are errors in the loading
+   * phase, for example.
+   */
+  public Collection<AspectValue> getSuccessfulAspects() {
+    return successfulAspects;
+  }
+
+  /**
    * See {@link #getSkippedTargets()}.
    */
   void setSkippedTargets(Collection<ConfiguredTarget> skippedTargets) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java
index d0b8b0c11c..005b1858a3 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java
@@ -66,92 +66,111 @@ class BuildResultPrinter {
     // problem where the summary message and the exit code disagree.  The logic
     // here is already complex.
 
+    OutErr outErr = request.getOutErr();
     Collection<ConfiguredTarget> targetsToPrint = filterTargetsToPrint(configuredTargets);
     Collection<AspectValue> aspectsToPrint = filterAspectsToPrint(aspects);
-
-    // Filter the targets we care about into two buckets:
-    Collection<ConfiguredTarget> succeeded = new ArrayList<>();
-    Collection<ConfiguredTarget> failed = new ArrayList<>();
-    for (ConfiguredTarget target : targetsToPrint) {
+    final boolean success;
+    if (aspectsToPrint.isEmpty()) {
+      // Suppress summary if --show_result value is exceeded:
+      if (targetsToPrint.size() > request.getBuildOptions().maxResultTargets) {
+        return;
+      }
+      // Filter the targets we care about into two buckets:
+      Collection<ConfiguredTarget> succeeded = new ArrayList<>();
+      Collection<ConfiguredTarget> failed = new ArrayList<>();
       Collection<ConfiguredTarget> successfulTargets = result.getSuccessfulTargets();
-      (successfulTargets.contains(target) ? succeeded : failed).add(target);
-    }
-
-    // TODO(bazel-team): convert these to a new "SKIPPED" status when ready: b/62191890.
-    failed.addAll(configuredTargetsToSkip);
-
-    // Suppress summary if --show_result value is exceeded:
-    if (succeeded.size() + failed.size() + aspectsToPrint.size()
-        > request.getBuildOptions().maxResultTargets) {
-      return;
-    }
+      for (ConfiguredTarget target : targetsToPrint) {
+        (successfulTargets.contains(target) ? succeeded : failed).add(target);
+      }
 
-    OutErr outErr = request.getOutErr();
+      // TODO(bazel-team): convert these to a new "SKIPPED" status when ready: b/62191890.
+      failed.addAll(configuredTargetsToSkip);
 
-    TopLevelArtifactContext context = request.getTopLevelArtifactContext();
-    for (ConfiguredTarget target : succeeded) {
-      Label label = target.getLabel();
-      // For up-to-date targets report generated artifacts, but only
-      // if they have associated action and not middleman artifacts.
-      boolean headerFlag = true;
-      for (Artifact artifact :
-          TopLevelArtifactHelper.getAllArtifactsToBuild(target, context).getImportantArtifacts()) {
-        if (shouldPrint(artifact)) {
-          if (headerFlag) {
-            outErr.printErr("Target " + label + " up-to-date:\n");
-            headerFlag = false;
+      TopLevelArtifactContext context = request.getTopLevelArtifactContext();
+      for (ConfiguredTarget target : succeeded) {
+        Label label = target.getLabel();
+        // For up-to-date targets report generated artifacts, but only
+        // if they have associated action and not middleman artifacts.
+        boolean headerFlag = true;
+        for (Artifact artifact :
+            TopLevelArtifactHelper.getAllArtifactsToBuild(target, context)
+                .getImportantArtifacts()) {
+          if (shouldPrint(artifact)) {
+            if (headerFlag) {
+              outErr.printErr("Target " + label + " up-to-date:\n");
+              headerFlag = false;
+            }
+            outErr.printErrLn(formatArtifactForShowResults(artifact, request));
           }
-          outErr.printErrLn(formatArtifactForShowResults(artifact, request));
         }
-      }
-      if (headerFlag) {
-        outErr.printErr("Target " + label + " up-to-date (nothing to build)\n");
-      }
-    }
-
-    for (AspectValue aspect : aspectsToPrint) {
-      Label label = aspect.getLabel();
-      String aspectName = aspect.getConfiguredAspect().getName();
-      boolean headerFlag = true;
-      NestedSet<Artifact> importantArtifacts =
-          TopLevelArtifactHelper.getAllArtifactsToBuild(aspect, context).getImportantArtifacts();
-      for (Artifact importantArtifact : importantArtifacts) {
         if (headerFlag) {
-          outErr.printErr("Aspect " + aspectName + " of " + label + " up-to-date:\n");
-          headerFlag = false;
+          outErr.printErr("Target " + label + " up-to-date (nothing to build)\n");
         }
-        if (shouldPrint(importantArtifact)) {
-          outErr.printErrLn(formatArtifactForShowResults(importantArtifact, request));
+      }
+      for (ConfiguredTarget target : failed) {
+        outErr.printErr("Target " + target.getLabel() + " failed to build\n");
+
+        // For failed compilation, it is still useful to examine temp artifacts,
+        // (ie, preprocessed and assembler files).
+        OutputGroupInfo topLevelProvider = OutputGroupInfo.get(target);
+        String productName = env.getRuntime().getProductName();
+        if (topLevelProvider != null) {
+          for (Artifact temp : topLevelProvider.getOutputGroup(OutputGroupInfo.TEMP_FILES)) {
+            if (temp.getPath().exists()) {
+              outErr.printErrLn(
+                  "  See temp at "
+                      + OutputDirectoryLinksUtils.getPrettyPath(
+                          temp.getPath(),
+                          env.getWorkspaceName(),
+                          env.getWorkspace(),
+                          request.getBuildOptions().getSymlinkPrefix(productName),
+                          productName));
+            }
+          }
         }
       }
-      if (headerFlag) {
-        outErr.printErr(
-            "Aspect " + aspectName + " of " + label + " up-to-date (nothing to build)\n");
+      success = failed.isEmpty();
+    } else {
+      // Suppress summary if --show_result value is exceeded:
+      if (aspectsToPrint.size() > request.getBuildOptions().maxResultTargets) {
+        return;
       }
-    }
-
-    for (ConfiguredTarget target : failed) {
-      outErr.printErr("Target " + target.getLabel() + " failed to build\n");
-
-      // For failed compilation, it is still useful to examine temp artifacts,
-      // (ie, preprocessed and assembler files).
-      OutputGroupInfo topLevelProvider =
-          OutputGroupInfo.get(target);
-      String productName = env.getRuntime().getProductName();
-      if (topLevelProvider != null) {
-        for (Artifact temp : topLevelProvider.getOutputGroup(OutputGroupInfo.TEMP_FILES)) {
-          if (temp.getPath().exists()) {
-            outErr.printErrLn("  See temp at "
-                + OutputDirectoryLinksUtils.getPrettyPath(temp.getPath(),
-                    env.getWorkspaceName(),
-                    env.getWorkspace(),
-                    request.getBuildOptions().getSymlinkPrefix(productName),
-                    productName));
+      // Filter the targets we care about into two buckets:
+      Collection<AspectValue> succeeded = new ArrayList<>();
+      Collection<AspectValue> failed = new ArrayList<>();
+      Collection<AspectValue> successfulAspects = result.getSuccessfulAspects();
+      for (AspectValue aspect : aspectsToPrint) {
+        (successfulAspects.contains(aspect) ? succeeded : failed).add(aspect);
+      }
+      TopLevelArtifactContext context = request.getTopLevelArtifactContext();
+      for (AspectValue aspect : succeeded) {
+        Label label = aspect.getLabel();
+        String aspectName = aspect.getConfiguredAspect().getName();
+        boolean headerFlag = true;
+        NestedSet<Artifact> importantArtifacts =
+            TopLevelArtifactHelper.getAllArtifactsToBuild(aspect, context).getImportantArtifacts();
+        for (Artifact importantArtifact : importantArtifacts) {
+          if (headerFlag) {
+            outErr.printErr("Aspect " + aspectName + " of " + label + " up-to-date:\n");
+            headerFlag = false;
+          }
+          if (shouldPrint(importantArtifact)) {
+            outErr.printErrLn(formatArtifactForShowResults(importantArtifact, request));
           }
         }
+        if (headerFlag) {
+          outErr.printErr(
+              "Aspect " + aspectName + " of " + label + " up-to-date (nothing to build)\n");
+        }
+      }
+      for (AspectValue aspect : failed) {
+        Label label = aspect.getLabel();
+        String aspectName = aspect.getConfiguredAspect().getName();
+        outErr.printErr("Aspect " + aspectName + " of " + label + " failed to build\n");
       }
+      success = failed.isEmpty();
     }
-    if (!failed.isEmpty() && !request.getOptions(ExecutionOptions.class).verboseFailures) {
+    if (!success && !request.getOptions(ExecutionOptions.class).verboseFailures) {
       outErr.printErr("Use --verbose_failures to see the command lines of failed build steps.\n");
     }
   }
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 ef4319573a..1becb5bc93 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
@@ -22,6 +22,9 @@ import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 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.AspectCompletionValue.AspectCompletionKey;
+import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor;
 import com.google.devtools.build.lib.skyframe.TargetCompletionValue;
@@ -47,6 +50,7 @@ public final class ExecutionProgressReceiver
 
   // Must be thread-safe!
   private final Set<ConfiguredTarget> builtTargets;
+  private final Set<AspectKey> builtAspects;
   private final Set<ActionLookupData> enqueuedActions = Sets.newConcurrentHashSet();
   private final Set<ActionLookupData> completedActions = Sets.newConcurrentHashSet();
   private final Set<ActionLookupData> ignoredActions = Sets.newConcurrentHashSet();
@@ -65,9 +69,9 @@ public final class ExecutionProgressReceiver
    * permitted while this receiver is active.
    */
   ExecutionProgressReceiver(
-      Set<ConfiguredTarget> builtTargets,
-      int exclusiveTestsCount) {
+      Set<ConfiguredTarget> builtTargets, Set<AspectKey> builtAspects, int exclusiveTestsCount) {
     this.builtTargets = Collections.synchronizedSet(builtTargets);
+    this.builtAspects = Collections.synchronizedSet(builtAspects);
     this.exclusiveTestsCount = exclusiveTestsCount;
   }
 
@@ -103,9 +107,15 @@ public final class ExecutionProgressReceiver
       if (value == null) {
         return;
       }
-
       ConfiguredTarget target = value.getConfiguredTarget();
       builtTargets.add(target);
+    } else if (type.equals(SkyFunctions.ASPECT_COMPLETION)) {
+      AspectCompletionValue value = (AspectCompletionValue) skyValueSupplier.get();
+      if (value == null) {
+        return;
+      }
+      AspectKey aspectKey = ((AspectCompletionKey) skyKey).aspectKey();
+      builtAspects.add(aspectKey);
     } else if (type.equals(SkyFunctions.ACTION_EXECUTION)) {
       // Remember all completed actions, even those in error, regardless of having been cached or
       // really executed.
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
index 3323f58f26..6971aaca39 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.buildtool;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
@@ -75,6 +74,7 @@ import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.skyframe.AspectValue;
+import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.Builder;
 import com.google.devtools.build.lib.skyframe.OutputService;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -295,6 +295,7 @@ public class ExecutionTool {
                                   request.getOptionsDescription());
 
     Set<ConfiguredTarget> builtTargets = new HashSet<>();
+    Set<AspectKey> builtAspects = new HashSet<>();
     Collection<AspectValue> aspects = analysisResult.getAspects();
 
     Iterable<Artifact> allArtifactsForProviders =
@@ -345,6 +346,7 @@ public class ExecutionTool {
           analysisResult.getAspects(),
           executor,
           builtTargets,
+          builtAspects,
           request.getBuildOptions().explanationPath != null,
           env.getBlazeWorkspace().getLastExecutionTimeRange(),
           topLevelArtifactContext);
@@ -377,9 +379,13 @@ public class ExecutionTool {
         saveActionCache(actionCache);
       }
 
+      env.getEventBus()
+          .post(new ExecutionPhaseCompleteEvent(timer.stop().elapsed(TimeUnit.MILLISECONDS)));
+
       try (AutoProfiler p = AutoProfiler.profiled("Show results", ProfilerTask.INFO)) {
         buildResult.setSuccessfulTargets(
-            determineSuccessfulTargets(configuredTargets, builtTargets, timer));
+            determineSuccessfulTargets(configuredTargets, builtTargets));
+        buildResult.setSuccessfulAspects(determineSuccessfulAspects(aspects, builtAspects));
         buildResult.setSkippedTargets(analysisResult.getTargetsToSkip());
         BuildResultPrinter buildResultPrinter = new BuildResultPrinter(env);
         buildResultPrinter.showBuildResult(request, buildResult, configuredTargets,
@@ -529,16 +535,13 @@ public class ExecutionTool {
   }
 
   /**
-   * Computes the result of the build. Sets the list of successful (up-to-date)
-   * targets in the request object.
+   * Computes the result of the build. Sets the list of successful (up-to-date) targets in the
+   * request object.
    *
-   * @param configuredTargets The configured targets whose artifacts are to be
-   *                          built.
-   * @param timer A timer that was started when the execution phase started.
+   * @param configuredTargets The configured targets whose artifacts are to be built.
    */
   private Collection<ConfiguredTarget> determineSuccessfulTargets(
-      Collection<ConfiguredTarget> configuredTargets, Set<ConfiguredTarget> builtTargets,
-      Stopwatch timer) {
+      Collection<ConfiguredTarget> configuredTargets, Set<ConfiguredTarget> builtTargets) {
     // Maintain the ordering by copying builtTargets into a LinkedHashSet in the same iteration
     // order as configuredTargets.
     Collection<ConfiguredTarget> successfulTargets = new LinkedHashSet<>();
@@ -547,11 +550,22 @@ public class ExecutionTool {
         successfulTargets.add(target);
       }
     }
-    env.getEventBus().post(
-        new ExecutionPhaseCompleteEvent(timer.stop().elapsed(MILLISECONDS)));
     return successfulTargets;
   }
 
+  private Collection<AspectValue> determineSuccessfulAspects(
+      Collection<AspectValue> aspects, Set<AspectKey> builtAspects) {
+    // Maintain the ordering by copying builtTargets into a LinkedHashSet in the same iteration
+    // order as configuredTargets.
+    Collection<AspectValue> successfulAspects = new LinkedHashSet<>();
+    for (AspectValue aspect : aspects) {
+      if (builtAspects.contains(aspect.getKey())) {
+        successfulAspects.add(aspect);
+      }
+    }
+    return successfulAspects;
+  }
+
   /** Get action cache if present or reload it from the on-disk cache. */
   private ActionCache getActionCache() throws LocalEnvironmentException {
     try {
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 e72eda9ed0..f557e62ffe 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
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.skyframe.ActionExecutionInactivityWatchdog;
 import com.google.devtools.build.lib.skyframe.AspectValue;
+import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.Builder;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.util.AbruptExitException;
@@ -103,6 +104,7 @@ public class SkyframeBuilder implements Builder {
       Collection<AspectValue> aspects,
       Executor executor,
       Set<ConfiguredTarget> builtTargets,
+      Set<AspectKey> builtAspects,
       boolean explain,
       @Nullable Range<Long> lastExecutionTimeRange,
       TopLevelArtifactContext topLevelArtifactContext)
@@ -114,6 +116,7 @@ public class SkyframeBuilder implements Builder {
     ExecutionProgressReceiver executionProgressReceiver =
         new ExecutionProgressReceiver(
             Preconditions.checkNotNull(builtTargets),
+            Preconditions.checkNotNull(builtAspects),
             countTestActions(exclusiveTests));
     skyframeExecutor
         .getEventBus()