Fix _validation output group merging (#23676)

While
cd72583
made it so that a rule and an aspect can both specify validation
outputs, it accidentally didn't actually merge the `_validation` output
groups.

By adding the missing merge logic and having the `ValidateTarget` aspect
depend on all other aspects, both a rule and an aspect returning a
validation output is now fully supported with
`--experimental_use_validation_aspect`.

Closes #23589.

PiperOrigin-RevId: 675294550
Change-Id: I581417bee223f0b6aedbc0ce71cae75cd6609ef7 
(cherry picked from commit 750f0a1)

Fixes #23664

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

@@ -321,6 +321,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_complete_event",
+        "//src/main/java/com/google/devtools/build/lib/analysis:aspect_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
         "//src/main/java/com/google/devtools/build/lib/analysis:build_info_event",

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

@@ -148,6 +148,9 @@
  */
 @Immutable
 public final class AspectCollection {
+  /** The name of the native aspect that collects validation outputs. */
+  public static final String VALIDATION_ASPECT_NAME = "ValidateTarget";
+
   /** aspects that should be visible to a dependency */
   private final ImmutableSet<AspectDeps> usedAspects;
 
@@ -284,11 +287,14 @@ public static AspectCollection create(Iterable<Aspect> aspectPath)
         ImmutableList.copyOf(aspectMap.entrySet()).reverse()) {
       for (AspectDescriptor depAspectDescriptor : deps.keySet()) {
         Aspect depAspect = aspectMap.get(depAspectDescriptor);
+        // As any aspect can add validation outputs, the special validation aspect that collects
+        // their outputs has to depend on all aspects.
         if (depAspect
                 .getDefinition()
                 .getRequiredProvidersForAspects()
                 .isSatisfiedBy(aspect.getValue().getDefinition().getAdvertisedProviders())
-            || depAspect.getDefinition().requires(aspect.getValue())) {
+            || depAspect.getDefinition().requires(aspect.getValue())
+            || depAspect.getAspectClass().getName().equals(VALIDATION_ASPECT_NAME)) {
           deps.get(depAspectDescriptor).add(aspect.getKey());
         }
       }

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

@@ -208,6 +208,9 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers) throws Dupl
             "Output group " + VALIDATION + " provided twice with incompatible depset orders");
       }
     }
+    if (!validationOutputs.isEmpty()) {
+      outputGroups.put(VALIDATION, validationOutputs.build());
+    }
     return createInternal(ImmutableMap.copyOf(outputGroups));
   }
 

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

@@ -22,6 +22,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.AnalysisOptions;
+import com.google.devtools.build.lib.analysis.AspectCollection;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
@@ -57,7 +58,6 @@
  * as --keep_going, --jobs, etc.
  */
 public class BuildRequest implements OptionsProvider {
-  public static final String VALIDATION_ASPECT_NAME = "ValidateTarget";
 
   private static final ImmutableList<Class<? extends OptionsBase>> MANDATORY_OPTIONS =
       ImmutableList.of(
@@ -429,8 +429,8 @@ public TopLevelArtifactContext getTopLevelArtifactContext() {
   public ImmutableList<String> getAspects() {
     List<String> aspects = getBuildOptions().aspects;
     ImmutableList.Builder<String> result = ImmutableList.<String>builder().addAll(aspects);
-    if (!aspects.contains(VALIDATION_ASPECT_NAME) && useValidationAspect()) {
-      result.add(VALIDATION_ASPECT_NAME);
+    if (!aspects.contains(AspectCollection.VALIDATION_ASPECT_NAME) && useValidationAspect()) {
+      result.add(AspectCollection.VALIDATION_ASPECT_NAME);
     }
     return result.build();
   }
@@ -453,7 +453,7 @@ public ImmutableMap<String, String> getAspectsParameters() throws ViewCreationFa
     }
   }
 
-  /** Whether {@value #VALIDATION_ASPECT_NAME} is in use. */
+  /** Whether {@value AspectCollection#VALIDATION_ASPECT_NAME} is in use. */
   public boolean useValidationAspect() {
     return validationMode() == OutputGroupInfo.ValidationMode.ASPECT;
   }

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

@@ -19,6 +19,7 @@
 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.analysis.AspectCollection;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
@@ -105,7 +106,8 @@ private boolean outputTargets(
       var aspectsToPrintBuilder = ImmutableSet.<AspectKey>builder();
       var validationAspectsBuilder = ImmutableList.<AspectKey>builder();
       for (AspectKey key : aspects.keySet()) {
-        if (Objects.equals(key.getAspectClass().getName(), BuildRequest.VALIDATION_ASPECT_NAME)) {
+        if (Objects.equals(
+            key.getAspectClass().getName(), AspectCollection.VALIDATION_ASPECT_NAME)) {
           validationAspectsBuilder.add(key);
         } else {
           aspectsToPrintBuilder.add(key);

src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD

@@ -36,6 +36,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:middleman_type",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis:aspect_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",

src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java

@@ -16,6 +16,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.AspectCollection;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.buildtool.BuildRequest;
@@ -241,7 +242,7 @@ private static DetailedExitCode analyzeTestResults(
     if (buildRequest.useValidationAspect()) {
       validatedTargets =
           buildResult.getSuccessfulAspects().stream()
-              .filter(key -> BuildRequest.VALIDATION_ASPECT_NAME.equals(key.getAspectName()))
+              .filter(key -> AspectCollection.VALIDATION_ASPECT_NAME.equals(key.getAspectName()))
               .map(AspectKey::getBaseConfiguredTargetKey)
               .collect(ImmutableSet.toImmutableSet());
     } else {

src/test/shell/integration/validation_actions_test.sh

@@ -511,9 +511,7 @@ function test_validation_actions_flags() {
   expect_log "Target //validation_actions:foo0 up-to-date:"
 }
 
-function test_validation_actions_in_rule_and_aspect() {
-  setup_test_project
-
+function setup_validation_tool_aspect() {
   mkdir -p aspect
   cat > aspect/BUILD <<'EOF'
 exports_files(["aspect_validation_tool"])
@@ -545,10 +543,45 @@ EOF
 echo "aspect validation output" > $1
 EOF
   chmod +x aspect/aspect_validation_tool
+}
+
+function test_validation_actions_in_rule_and_aspect_no_use_validation_aspect() {
+  setup_test_project
+  setup_validation_tool_aspect
+  setup_passing_validation_action
+
+  bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \
+      --noexperimental_use_validation_aspect \
+      //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed"
+  expect_log "Target //validation_actions:foo0 up-to-date:"
+  expect_log "validation_actions/foo0.main"
+  assert_exists bazel-bin/validation_actions/foo0.validation
+  assert_exists bazel-bin/validation_actions/foo0.aspect_validation
+
+  cat > aspect/aspect_validation_tool <<'EOF'
+#!/bin/bash
+echo "aspect validation failed!"
+exit 1
+EOF
+
+  bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \
+      --noexperimental_use_validation_aspect \
+      //validation_actions:foo0 >& "$TEST_log" && fail "Expected build to fail"
+  expect_log "aspect validation failed!"
+}
+
+function test_validation_actions_in_rule_and_aspect_use_validation_aspect() {
+  setup_test_project
+  setup_validation_tool_aspect
   setup_passing_validation_action
 
   bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \
+      --experimental_use_validation_aspect \
       //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed"
+  expect_log "Target //validation_actions:foo0 up-to-date:"
+  expect_log "validation_actions/foo0.main"
+  assert_exists bazel-bin/validation_actions/foo0.validation
+  assert_exists bazel-bin/validation_actions/foo0.aspect_validation
 
   cat > aspect/aspect_validation_tool <<'EOF'
 #!/bin/bash
@@ -557,6 +590,7 @@ exit 1
 EOF
 
   bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \
+      --experimental_use_validation_aspect \
       //validation_actions:foo0 >& "$TEST_log" && fail "Expected build to fail"
   expect_log "aspect validation failed!"
 }