Use aspect to avoid validation actions blocking test executions

PiperOrigin-RevId: 369890289

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

@@ -16,17 +16,13 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CompletionContext;
 import com.google.devtools.build.lib.actions.EventReportingArtifacts;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
-import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
-import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.OutputGroup;
 import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint;
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
 import com.google.devtools.build.lib.causes.Cause;
@@ -158,31 +154,20 @@ public Collection<BuildEventId> getChildrenEvents() {
 
   @Override
   public ReportedArtifacts reportedArtifacts() {
-    ImmutableSet.Builder<NestedSet<Artifact>> builder = ImmutableSet.builder();
-    if (artifactOutputGroups != null) {
-      for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups.values()) {
-        builder.add(artifactsInGroup.getArtifacts());
-      }
-    }
-    return new ReportedArtifacts(builder.build(), completionContext);
+    return TargetCompleteEvent.toReportedArtifacts(
+        artifactOutputGroups, completionContext, /*baselineCoverageArtifacts=*/ null);
   }
 
   @Override
   public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
-    ArtifactGroupNamer namer = converters.artifactGroupNamer();
-
     BuildEventStreamProtos.TargetComplete.Builder builder =
         BuildEventStreamProtos.TargetComplete.newBuilder();
     builder.setSuccess(!failed());
-    if (artifactOutputGroups != null) {
-      artifactOutputGroups.forEach(
-          (outputGroup, artifactsInGroup) ->
-              builder.addOutputGroup(
-                  OutputGroup.newBuilder()
-                      .setName(outputGroup)
-                      .setIncomplete(artifactsInGroup.isIncomplete())
-                      .addFileSets(namer.apply(artifactsInGroup.getArtifacts().toNode()))));
-    }
+    builder.addAllOutputGroup(
+        TargetCompleteEvent.toOutputGroupProtos(
+            artifactOutputGroups,
+            converters.artifactGroupNamer(),
+            /*baselineCoverageArtifacts=*/ null));
     return GenericBuildEvent.protoChaining(this).setCompleted(builder.build()).build();
   }
 }

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

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -112,6 +113,10 @@ public final class OutputGroupInfo extends StructImpl
    */
   public static final String VALIDATION = HIDDEN_OUTPUT_GROUP_PREFIX + "validation";
 
+  /** Helper output group used to request {@link #VALIDATION} outputs from top-level aspect. */
+  public static final String VALIDATION_TOP_LEVEL =
+      HIDDEN_OUTPUT_GROUP_PREFIX + "validation_top_level" + INTERNAL_SUFFIX;
+
   /**
    * Temporary files created during building a rule, for example, .i, .d and .s files for C++
    * compilation.
@@ -134,6 +139,21 @@ public final class OutputGroupInfo extends StructImpl
   public static final ImmutableSortedSet<String> DEFAULT_GROUPS =
       ImmutableSortedSet.of(DEFAULT, TEMP_FILES, HIDDEN_TOP_LEVEL);
 
+  /** Request parameter for {@link #determineOutputGroups}. */
+  public enum ValidationMode {
+    /** Validation outputs not built. */
+    OFF,
+    /**
+     * Validation outputs built by requesting {@link #VALIDATION} output group Blaze core collects.
+     */
+    OUTPUT_GROUP,
+    /**
+     * Validation outputs built by {@code ValidateTarget} aspect "promoting" {@link #VALIDATION}
+     * output group Blaze core collects to {@link #VALIDATION_TOP_LEVEL} and requesting the latter.
+     */
+    ASPECT;
+  }
+
   private final ImmutableMap<String, NestedSet<Artifact>> outputGroups;
 
   public OutputGroupInfo(ImmutableMap<String, NestedSet<Artifact>> outputGroups) {
@@ -198,14 +218,13 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers)
   }
 
   public static ImmutableSortedSet<String> determineOutputGroups(
-      List<String> outputGroups, boolean includeValidationOutputGroup) {
-    return determineOutputGroups(DEFAULT_GROUPS, outputGroups, includeValidationOutputGroup);
+      List<String> outputGroups, ValidationMode validationMode) {
+    return determineOutputGroups(DEFAULT_GROUPS, outputGroups, validationMode);
   }
 
-  public static ImmutableSortedSet<String> determineOutputGroups(
-      Set<String> defaultOutputGroups,
-      List<String> outputGroups,
-      boolean includeValidationOutputGroup) {
+  @VisibleForTesting
+  static ImmutableSortedSet<String> determineOutputGroups(
+      Set<String> defaultOutputGroups, List<String> outputGroups, ValidationMode validationMode) {
 
     Set<String> current = Sets.newHashSet();
 
@@ -235,8 +254,14 @@ public static ImmutableSortedSet<String> determineOutputGroups(
     }
 
     // Add the validation output group regardless of the additions and subtractions above.
-    if (includeValidationOutputGroup) {
-      current.add(VALIDATION);
+    switch (validationMode) {
+      case OUTPUT_GROUP:
+        current.add(VALIDATION);
+        break;
+      case ASPECT:
+        current.add(VALIDATION_TOP_LEVEL);
+        break;
+      case OFF: // fall out
     }
 
     return ImmutableSortedSet.copyOf(current);

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

@@ -470,6 +470,13 @@ public ImmutableList<BuildEventId> postedAfter() {
 
   @Override
   public ReportedArtifacts reportedArtifacts() {
+    return toReportedArtifacts(outputs, completionContext, baselineCoverageArtifacts);
+  }
+
+  static ReportedArtifacts toReportedArtifacts(
+      ImmutableMap<String, ArtifactsInOutputGroup> outputs,
+      CompletionContext completionContext,
+      @Nullable NestedSet<Artifact> baselineCoverageArtifacts) {
     ImmutableSet.Builder<NestedSet<Artifact>> builder = ImmutableSet.builder();
     for (ArtifactsInOutputGroup artifactsInGroup : outputs.values()) {
       if (artifactsInGroup.areImportant()) {
@@ -492,6 +499,14 @@ private Iterable<String> getTags() {
   }
 
   private Iterable<OutputGroup> getOutputFilesByGroup(ArtifactGroupNamer namer) {
+    return toOutputGroupProtos(outputs, namer, baselineCoverageArtifacts);
+  }
+
+  /** Returns {@link OutputGroup} protos for given output groups and optional coverage artifacts. */
+  static ImmutableList<OutputGroup> toOutputGroupProtos(
+      ImmutableMap<String, ArtifactsInOutputGroup> outputs,
+      ArtifactGroupNamer namer,
+      @Nullable NestedSet<Artifact> baselineCoverageArtifacts) {
     ImmutableList.Builder<OutputGroup> groups = ImmutableList.builder();
     outputs.forEach(
         (outputGroup, artifactsInOutputGroup) -> {

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

@@ -50,6 +50,8 @@
  * as --keep_going, --jobs, etc.
  */
 public class BuildRequest implements OptionsProvider {
+  static final String VALIDATION_ASPECT_NAME = "ValidateTarget";
+
   private static final ImmutableList<Class<? extends OptionsBase>> MANDATORY_OPTIONS =
       ImmutableList.of(
           BuildRequestOptions.class,
@@ -371,16 +373,35 @@ public TopLevelArtifactContext getTopLevelArtifactContext() {
         getOptions(ExecutionOptions.class).testStrategy.equals("exclusive"),
         getOptions(BuildEventProtocolOptions.class).expandFilesets,
         getOptions(BuildEventProtocolOptions.class).fullyResolveFilesetSymlinks,
-        OutputGroupInfo.determineOutputGroups(
-            buildOptions.outputGroups, buildOptions.runValidationActions));
+        OutputGroupInfo.determineOutputGroups(buildOptions.outputGroups, validationMode()));
   }
 
   public ImmutableSortedSet<String> getMultiCpus() {
     return ImmutableSortedSet.copyOf(getBuildOptions().multiCpus);
   }
 
   public ImmutableList<String> getAspects() {
-    return ImmutableList.copyOf(getBuildOptions().aspects);
+    ImmutableList.Builder<String> result =
+        ImmutableList.<String>builder().addAll(getBuildOptions().aspects);
+    if (useValidationAspect()) {
+      result.add(VALIDATION_ASPECT_NAME);
+    }
+    return result.build();
+  }
+
+  /** Whether {@value #VALIDATION_ASPECT_NAME} is in use. */
+  boolean useValidationAspect() {
+    return validationMode() == OutputGroupInfo.ValidationMode.ASPECT;
+  }
+
+  private OutputGroupInfo.ValidationMode validationMode() {
+    BuildRequestOptions buildOptions = getBuildOptions();
+    if (!buildOptions.runValidationActions) {
+      return OutputGroupInfo.ValidationMode.OFF;
+    }
+    return buildOptions.useValidationAspect
+        ? OutputGroupInfo.ValidationMode.ASPECT
+        : OutputGroupInfo.ValidationMode.OUTPUT_GROUP;
   }
 
   public boolean getCheckForActionConflicts() {

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

@@ -160,6 +160,14 @@ public class BuildRequestOptions extends OptionsBase {
       help = "Whether to run validation actions as part of the build.")
   public boolean runValidationActions;
 
+  @Option(
+      name = "experimental_use_validation_aspect",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
+      effectTags = {OptionEffectTag.EXECUTION, OptionEffectTag.AFFECTS_OUTPUTS},
+      help = "Whether to run validation actions using aspect (for parallelism with tests).")
+  public boolean useValidationAspect;
+
   @Option(
       name = "show_result",
       defaultValue = "1",

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

@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.util.io.OutErr;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -78,23 +79,51 @@ void showBuildResult(
                 : env.getWorkspace(),
             request.getBuildOptions().experimentalNoProductNameOutSymlink);
     OutErr outErr = request.getOutErr();
-    Collection<ConfiguredTarget> targetsToPrint = filterTargetsToPrint(configuredTargets);
+
+    // Produce output as if validation aspect didn't exist; instead we'll consult validation aspect
+    // if we end up printing targets below. Note that in the presence of other aspects, we may print
+    // success messages for them but the overall build will still fail if validation aspects (or
+    // targets) failed.
+    Collection<AspectKey> aspectsToPrint = aspects.keySet();
+    if (request.useValidationAspect()) {
+      aspectsToPrint =
+          aspectsToPrint.stream()
+              .filter(
+                  k -> !BuildRequest.VALIDATION_ASPECT_NAME.equals(k.getAspectClass().getName()))
+              .collect(ImmutableList.toImmutableList());
+    }
     final boolean success;
-    if (aspects.isEmpty()) {
+    if (aspectsToPrint.isEmpty()) {
       // Suppress summary if --show_result value is exceeded:
+      Collection<ConfiguredTarget> targetsToPrint = filterTargetsToPrint(configuredTargets);
       if (targetsToPrint.size() > request.getBuildOptions().maxResultTargets) {
         return;
       }
-      // Filter the targets we care about into three buckets:
+
+      // Filter the targets we care about into three buckets. Targets are only considered successful
+      // if they and their validation aspects succeeded. Note we determined above that all aspects
+      // are validation aspects, so just use the full keySet() here.
+      ImmutableMap<ConfiguredTargetKey, Boolean> validated =
+          aspects.keySet().stream()
+              .collect(
+                  ImmutableMap.toImmutableMap(
+                      AspectKey::getBaseConfiguredTargetKey,
+                      k -> result.getSuccessfulAspects().contains(k),
+                      Boolean::logicalAnd));
+
       Collection<ConfiguredTarget> succeeded = new ArrayList<>();
       Collection<ConfiguredTarget> failed = new ArrayList<>();
       Collection<ConfiguredTarget> skipped = new ArrayList<>();
       Collection<ConfiguredTarget> successfulTargets = result.getSuccessfulTargets();
       for (ConfiguredTarget target : targetsToPrint) {
         if (configuredTargetsToSkip.contains(target)) {
           skipped.add(target);
+        } else if (successfulTargets.contains(target)
+            && validated.getOrDefault(
+                ConfiguredTargetKey.builder().setConfiguredTarget(target).build(), Boolean.TRUE)) {
+          succeeded.add(target);
         } else {
-          (successfulTargets.contains(target) ? succeeded : failed).add(target);
+          failed.add(target);
         }
       }
 
@@ -142,14 +171,14 @@ void showBuildResult(
       success = failed.isEmpty();
     } else {
       // Suppress summary if --show_result value is exceeded:
-      if (aspects.size() > request.getBuildOptions().maxResultTargets) {
+      if (aspectsToPrint.size() > request.getBuildOptions().maxResultTargets) {
         return;
       }
       // Filter the targets we care about into two buckets:
       Collection<AspectKey> succeeded = new ArrayList<>();
       Collection<AspectKey> failed = new ArrayList<>();
       ImmutableSet<AspectKey> successfulAspects = result.getSuccessfulAspects();
-      for (AspectKey aspect : aspects.keySet()) {
+      for (AspectKey aspect : aspectsToPrint) {
         (successfulAspects.contains(aspect) ? succeeded : failed).add(aspect);
       }
       TopLevelArtifactContext context = request.getTopLevelArtifactContext();

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

@@ -57,9 +57,14 @@ java_library(
         ["core/*.java"],
     ),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory",
+        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
+        "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//third_party:guava",
     ],
 )

src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java

@@ -39,6 +39,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
     builder.addRuleDefinition(new BaseRuleClasses.MakeVariableExpandingRule());
     builder.addRuleDefinition(new BaseRuleClasses.BinaryBaseRule());
     builder.addRuleDefinition(new BaseRuleClasses.TestBaseRule());
+    builder.addNativeAspectClass(new ValidateTarget()); // internally used aspect
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/rules/core/ValidateTarget.java

@@ -0,0 +1,63 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.rules.core;
+
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
+import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
+import com.google.devtools.build.lib.analysis.OutputGroupInfo;
+import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.packages.AspectDefinition;
+import com.google.devtools.build.lib.packages.AspectParameters;
+import com.google.devtools.build.lib.packages.NativeAspectClass;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+
+/**
+ * Non-recursive aspect that promotes {@link OutputGroupInfo#VALIDATION} outputs to {@link
+ * OutputGroupInfo#VALIDATION_TOP_LEVEL}. By requesting the latter but not the former output group,
+ * validations avoid blocking test execution. (Using {@link OutputGroupInfo#DEFAULT} would
+ * accomplish that as well but would be overrideable with {@code --output_groups} flag.)
+ *
+ * <p>Name is chosen to make for semi-sensible "ValidateTarget" aspect events.
+ */
+class ValidateTarget extends NativeAspectClass implements ConfiguredAspectFactory {
+
+  @Override
+  public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+    return AspectDefinition.builder(this)
+        .applyToFiles(true) // to grab validation outputs from file targets
+        .build();
+  }
+
+  @Override
+  public ConfiguredAspect create(
+      ConfiguredTargetAndData ctadBase,
+      RuleContext context,
+      AspectParameters parameters,
+      String toolsRepository)
+      throws ActionConflictException, InterruptedException {
+    OutputGroupInfo outputGroupInfo = OutputGroupInfo.get(ctadBase.getConfiguredTarget());
+    if (outputGroupInfo != null) {
+      NestedSet<Artifact> validations = outputGroupInfo.getOutputGroup(OutputGroupInfo.VALIDATION);
+      if (!validations.isEmpty()) {
+        return ConfiguredAspect.builder(context)
+            .addOutputGroup(OutputGroupInfo.VALIDATION_TOP_LEVEL, validations)
+            .build();
+      }
+    }
+    return ConfiguredAspect.forNonapplicableTarget();
+  }
+}