Repository: bazelbuild/bazel
Issue: Aspects: Merge validation output group
Labels: type: feature request
untriaged
team-Core

### Description of the feature request:


If I have a rule that returns the special [_validation](https://bazel.build/extending/rules#validation_actions) output group, it's not possible for an aspect also return the `_validation` output group. This results in a `Output group _validation provided twice` error.

According to the [aspect documentation](https://bazel.build/extending/aspects):
> It is an error if a target and an aspect that is applied to it each provide a provider with the same type, with the exceptions of [OutputGroupInfo](https://bazel.build/rules/lib/providers/OutputGroupInfo) (which is merged, so long as the rule and aspect specify different output groups)

It would be nice to extend this allow merging the `_validation` output group.

### Which category does this issue belong to?


Core

### What underlying problem are you trying to solve with this feature?


Allow multiple aspects to declare their own validations.

### Which operating system are you running Bazel on?


Linux

### What is the output of `bazel info release`?


6.2.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

https://groups.google.com/g/bazel-discuss/c/TCo0msh1My4 was used as an inspiration to add validations via aspects.

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
A fix for this issue has been included in [Bazel 6.4.0 RC2](https://releases.bazel.build/6.4.0/rc2/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!

Pull request review comments:

Patch that resolved the issue:
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());
   }
 


Test patch:
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"