From e3c27b635152d9007a01acbfea713c25af599cbe Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 5 Oct 2023 11:41:47 -0700
Subject: [PATCH] Merge rule and aspect validation output groups

By merging the special `_validation` output groups across a rule and its attached aspects, aspects and rules can simultaneously use validation actions.

Fixes #19624

Closes #19630.

PiperOrigin-RevId: 571084964
Change-Id: I3135ce1f9e61124e96a3b7d8e0ea0c3a3cdf526d

Fixes #19742
---
 .../build/lib/analysis/OutputGroupInfo.java   | 17 ++++++-
 .../integration/validation_actions_test.sh    | 50 +++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java
index 44720d8c8d9352..f98b8e49853a54 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java
@@ -189,7 +189,8 @@ public NestedSet<Artifact> getOutputGroup(String outputGroupName) {
   }
 
   /**
-   * Merges output groups from two output providers. The set of output groups must be disjoint.
+   * Merges output groups from two output providers. The set of output groups must be disjoint,
+   * except for the special validation output group, which is always merged.
    *
    * @param providers providers to merge {@code this} with.
    */
@@ -207,6 +208,9 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers)
     Set<String> seenGroups = new HashSet<>();
     for (OutputGroupInfo provider : providers) {
       for (String outputGroup : provider.outputGroups.keySet()) {
+        if (outputGroup.equals(VALIDATION)) {
+          continue;
+        }
         if (!seenGroups.add(outputGroup)) {
           throw new DuplicateException(
               "Output group " + outputGroup + " provided twice");
@@ -215,6 +219,17 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers)
         resultBuilder.put(outputGroup, provider.getOutputGroup(outputGroup));
       }
     }
+    // Allow both an aspect and the rule to use validation actions.
+    NestedSetBuilder<Artifact> validationOutputs = NestedSetBuilder.stableOrder();
+    for (OutputGroupInfo provider : providers) {
+      try {
+        validationOutputs.addTransitive(provider.getOutputGroup(VALIDATION));
+      } catch (IllegalArgumentException e) {
+        // Thrown if nested set orders aren't compatible.
+        throw new DuplicateException(
+            "Output group " + VALIDATION + " provided twice with incompatible depset orders");
+      }
+    }
     return new OutputGroupInfo(resultBuilder.buildOrThrow());
   }
 
diff --git a/src/test/shell/integration/validation_actions_test.sh b/src/test/shell/integration/validation_actions_test.sh
index 6145c1f2b284a6..0790c9161fe4bb 100755
--- a/src/test/shell/integration/validation_actions_test.sh
+++ b/src/test/shell/integration/validation_actions_test.sh
@@ -508,4 +508,54 @@ 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
+
+  mkdir -p aspect
+  cat > aspect/BUILD <<'EOF'
+exports_files(["aspect_validation_tool"])
+EOF
+  cat > aspect/def.bzl <<'EOF'
+def _validation_aspect_impl(target, ctx):
+  validation_output = ctx.actions.declare_file(ctx.rule.attr.name + ".aspect_validation")
+  ctx.actions.run(
+      outputs = [validation_output],
+      executable = ctx.executable._validation_tool,
+      arguments = [validation_output.path])
+  return [
+    OutputGroupInfo(_validation = depset([validation_output])),
+  ]
+
+validation_aspect = aspect(
+  implementation = _validation_aspect_impl,
+  attrs = {
+    "_validation_tool": attr.label(
+        allow_single_file = True,
+        default = Label(":aspect_validation_tool"),
+        executable = True,
+        cfg = "exec"),
+  },
+)
+EOF
+  cat > aspect/aspect_validation_tool <<'EOF'
+#!/bin/bash
+echo "aspect validation output" > $1
+EOF
+  chmod +x aspect/aspect_validation_tool
+  setup_passing_validation_action
+
+  bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \
+      //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed"
+
+  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 \
+      //validation_actions:foo0 >& "$TEST_log" && fail "Expected build to fail"
+  expect_log "aspect validation failed!"
+}
+
 run_suite "Validation actions integration tests"