Add aspect data to action conflict message and add a test that exhibi…

…ts current Bazel behavior: we emit an error message on an action conflict even if the relevant output files are never requested during the build.

PiperOrigin-RevId: 407146827

src/main/java/com/google/devtools/build/lib/actions/MutableActionGraph.java

@@ -24,12 +24,15 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.server.FailureDetails.Analysis;
 import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.skyframe.SaneAnalysisException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import java.util.Set;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
 
 /**
  * A mutable action graph. Implementations of this interface must be thread-safe.
@@ -186,6 +189,10 @@ private static String debugSuffix(
 
       addStringDetail(sb, "Label", aNull ? null : Label.print(aOwner.getLabel()),
           bNull ? null : Label.print(bOwner.getLabel()));
+      if ((!aNull && !aOwner.getAspectDescriptors().isEmpty())
+          || (!bNull && !bOwner.getAspectDescriptors().isEmpty())) {
+        addStringDetail(sb, "Aspects", aspectDescriptor(aOwner), aspectDescriptor(bOwner));
+      }
       addStringDetail(sb, "RuleClass", aNull ? null : aOwner.getTargetKind(),
           bNull ? null : bOwner.getTargetKind());
       addStringDetail(sb, "Configuration", aNull ? null : aOwner.getConfigurationChecksum(),
@@ -250,5 +257,14 @@ private static String debugSuffix(
 
       return sb.toString();
     }
+
+    @Nullable
+    private static String aspectDescriptor(ActionOwner owner) {
+      return owner == null
+          ? null
+          : owner.getAspectDescriptors().stream()
+              .map(AspectDescriptor::getDescription)
+              .collect(Collectors.joining(",", "[", "]"));
+    }
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java

@@ -42,6 +42,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.OutputFilter.RegexOutputFilter;
 import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.AspectParameters;
@@ -51,6 +52,7 @@
 import com.google.devtools.build.lib.packages.StarlarkProvider;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Root;
@@ -965,6 +967,11 @@ public void topLevelConflictDetected() throws Exception {
         .containsMatch(
             "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect .',"
                 + " attempted action: action 'Action for aspect .'");
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        Pattern.compile(
+            "Aspects: \\[//foo:aspect.bzl%aspect[12]], \\[//foo:aspect.bzl%aspect[12]]"),
+        EventKind.ERROR);
 
     // Fix bzl file so actions are shared: analysis should succeed now.
     scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "1"));
@@ -997,6 +1004,47 @@ public void topLevelConflictDetected() throws Exception {
                 + " attempted action: action 'Action for aspect .'");
   }
 
+  @Test
+  public void conflictBetweenTargetAndAspect() throws Exception {
+    scratch.file(
+        "foo/aspect.bzl",
+        "def _aspect_impl(target, ctx):",
+        "  outfile = ctx.actions.declare_file('conflict.out')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [outfile],",
+        "    progress_message = 'Action for aspect',",
+        "    command = 'echo \"aspect\" > ' + outfile.path,",
+        "  )",
+        "  return [OutputGroupInfo(files = [outfile])]",
+        "",
+        "def _rule_impl(ctx):",
+        "  outfile = ctx.actions.declare_file('conflict.out')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [outfile],",
+        "    progress_message = 'Action for target',",
+        "    command = 'echo \"target\" > ' + outfile.path,",
+        "  )",
+        "  return [DefaultInfo(files = depset([outfile]))]",
+        "my_aspect = aspect(implementation = _aspect_impl)",
+        "my_rule = rule(implementation = _rule_impl, attrs = {'deps' : attr.label_list(aspects ="
+            + " [my_aspect])})");
+    scratch.file(
+        "foo/BUILD",
+        "load('//foo:aspect.bzl', 'my_aspect', 'my_rule')",
+        "my_rule(name = 'foo', deps = [':dep'])",
+        "sh_library(name = 'dep', srcs = ['dep.sh'])");
+    // Expect errors.
+    reporter.removeHandler(failFastHandler);
+    ViewCreationFailedException exception =
+        assertThrows(ViewCreationFailedException.class, () -> update("//foo:foo"));
+    assertThat(exception).hasMessageThat().containsMatch("ConflictException: for foo/conflict.out");
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        Pattern.compile(
+            "Aspects: (\\[], \\[//foo:aspect.bzl%my_aspect]|\\[//foo:aspect.bzl%my_aspect], \\[])"),
+        EventKind.ERROR);
+  }
+
   @Test
   public void aspectDuplicatesRuleProviderError() throws Exception {
     setRulesAndAspectsAvailableInTests(ImmutableList.of(), ImmutableList.of());

src/test/java/com/google/devtools/build/lib/buildtool/BUILD

@@ -433,9 +433,11 @@ java_test(
     deps = [
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//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:view_creation_failed_exception",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
+        "//src/main/protobuf:failure_details_java_proto",
         "//src/test/java/com/google/devtools/build/lib/buildtool/util",
         "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
         "//third_party:guava",

src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java

@@ -20,6 +20,7 @@
 
 import com.google.common.collect.Iterables;
 import com.google.common.eventbus.Subscribe;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.BuildFailedException;
 import com.google.devtools.build.lib.actions.MutableActionGraph;
 import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
@@ -29,10 +30,12 @@
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.regex.Pattern;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -374,6 +377,75 @@ public void testConflictErrorAndAnalysisError() throws Exception {
         .containsAnyOf("//conflict:x", "//conflict:_objs/x/foo.pic.o");
   }
 
+  // This test is documenting current behavior more than enforcing a contract: it might be ok for
+  // Bazel to suppress the error message about an action conflict, since the relevant actions are
+  // not run in this build. However, that might cause problems for users who aren't immediately
+  // alerted when they introduce an action conflict. We already skip exhaustive checks for action
+  // conflicts in the name of performance and that has prompted complaints, so suppressing actual
+  // conflicts seems like a bad idea.
+  //
+  // While this test is written with aspects, any actions that generate conflicting outputs but
+  // aren't run would exhibit this behavior.
+  @Test
+  public void unusedActionsStillConflict() throws Exception {
+    write(
+        "foo/aspect.bzl",
+        "def _aspect1_impl(target, ctx):",
+        "  outfile = ctx.actions.declare_file('aspect.out')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [outfile],",
+        "    progress_message = 'Action for aspect 1',",
+        "    command = 'echo \"1\" > ' + outfile.path,",
+        "  )",
+        "  return [OutputGroupInfo(files1 = [outfile])]",
+        "",
+        "def _aspect2_impl(target, ctx):",
+        "  outfile = ctx.actions.declare_file('aspect.out')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [outfile],",
+        "    progress_message = 'Action for aspect 2',",
+        "    command = 'echo \"2\" > ' + outfile.path,",
+        "  )",
+        "  return [OutputGroupInfo(files2 = [outfile])]",
+        "",
+        "def _rule_impl(ctx):",
+        "  outfile = ctx.actions.declare_file('file.out')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [outfile],",
+        "    progress_message = 'Action for target',",
+        "    command = 'touch ' + outfile.path,",
+        "  )",
+        "  return [DefaultInfo(files = depset([outfile]))]",
+        "aspect1 = aspect(implementation = _aspect1_impl)",
+        "aspect2 = aspect(implementation = _aspect2_impl)",
+        "",
+        "bad_rule = rule(implementation = _rule_impl, attrs = {'deps' : attr.label_list(aspects ="
+            + " [aspect1, aspect2])})");
+    write(
+        "foo/BUILD",
+        "load('//foo:aspect.bzl', 'bad_rule')",
+        "sh_library(name = 'dep', srcs = ['dep.sh'])",
+        "bad_rule(name = 'foo', deps = [':dep'])");
+    addOptions("--keep_going");
+    // If Bazel decides to permit this scenario, the build should succeed instead of throwing here.
+    BuildFailedException buildFailedException =
+        assertThrows(BuildFailedException.class, () -> buildTarget("//foo:foo"));
+    assertThat(buildFailedException)
+        .hasMessageThat()
+        .contains("command succeeded, but not all targets were analyzed");
+    // We successfully built the output file despite the supposed failure.
+    Iterable<Artifact> artifacts = getArtifacts("//foo:foo");
+    assertThat(artifacts).hasSize(1);
+    assertThat(Iterables.getOnlyElement(artifacts).getPath().exists()).isTrue();
+    assertThat(
+            buildFailedException.getDetailedExitCode().getFailureDetail().getAnalysis().getCode())
+        .isEqualTo(FailureDetails.Analysis.Code.NOT_ALL_TARGETS_ANALYZED);
+    events.assertContainsError("file 'foo/aspect.out' is generated by these conflicting actions:");
+    events.assertContainsError(
+        Pattern.compile(
+            "Aspects: \\[//foo:aspect.bzl%aspect[12]], \\[//foo:aspect.bzl%aspect[12]]"));
+  }
+
   @Test
   public void testMultipleConflictErrors() throws Exception {
     write(