Automated rollback of commit 750f0a1.

*** Reason for rollback ***

This CL breaks internal tests.

*** Original change description ***

Fix `_validation` output group merging

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 f...

***

PiperOrigin-RevId: 675632532
Change-Id: I25f19f22f5dbcce19ea801e88e9ba8595afcf428

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

@@ -333,7 +333,6 @@ 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,9 +148,6 @@
  */
 @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;
 
@@ -286,14 +283,11 @@ 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.getAspectClass().getName().equals(VALIDATION_ASPECT_NAME)) {
+            || depAspect.getDefinition().requires(aspect.getValue())) {
           deps.get(depAspectDescriptor).add(aspect.getKey());
         }
       }

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

@@ -209,9 +209,6 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers) throws Merg
             "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,7 +22,6 @@
 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;
@@ -58,6 +57,7 @@
  * 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(AspectCollection.VALIDATION_ASPECT_NAME) && useValidationAspect()) {
-      result.add(AspectCollection.VALIDATION_ASPECT_NAME);
+    if (!aspects.contains(VALIDATION_ASPECT_NAME) && useValidationAspect()) {
+      result.add(VALIDATION_ASPECT_NAME);
     }
     return result.build();
   }
@@ -453,7 +453,7 @@ public ImmutableMap<String, String> getAspectsParameters() throws ViewCreationFa
     }
   }
 
-  /** Whether {@value AspectCollection#VALIDATION_ASPECT_NAME} is in use. */
+  /** Whether {@value #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,7 +19,6 @@
 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;
@@ -390,7 +389,7 @@ private static PartitionedAspectKeys partitionAspectKeys(
     var aspectsToPrintBuilder = ImmutableSet.<AspectKey>builder();
     var validationAspectsBuilder = ImmutableList.<AspectKey>builder();
     for (AspectKey key : keys) {
-      if (Objects.equals(key.getAspectClass().getName(), AspectCollection.VALIDATION_ASPECT_NAME)) {
+      if (Objects.equals(key.getAspectClass().getName(), BuildRequest.VALIDATION_ASPECT_NAME)) {
         validationAspectsBuilder.add(key);
       } else {
         aspectsToPrintBuilder.add(key);

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

@@ -36,7 +36,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//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

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 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;
@@ -244,7 +243,7 @@ private static DetailedExitCode analyzeTestResults(
     if (buildRequest.useValidationAspect()) {
       validatedTargets =
           buildResult.getSuccessfulAspects().stream()
-              .filter(key -> AspectCollection.VALIDATION_ASPECT_NAME.equals(key.getAspectName()))
+              .filter(key -> BuildRequest.VALIDATION_ASPECT_NAME.equals(key.getAspectName()))
               .map(AspectKey::getBaseConfiguredTargetKey)
               .collect(ImmutableSet.toImmutableSet());
     } else {

src/test/shell/integration/validation_actions_test.sh

@@ -519,7 +519,9 @@ function test_validation_actions_flags() {
   expect_log "Target //validation_actions:foo0 up-to-date:"
 }
 
-function setup_validation_tool_aspect() {
+function test_validation_actions_in_rule_and_aspect() {
+  setup_test_project
+
   mkdir -p aspect
   cat > aspect/BUILD <<'EOF'
 exports_files(["aspect_validation_tool"])
@@ -551,45 +553,10 @@ 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
@@ -598,7 +565,6 @@ 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!"
 }