Fix ActionConflictException message

PiperOrigin-RevId: 578208178 Change-Id: Idbebf35c029f8748f57caab3519a73b10b6b06ca
bazel-io · Oct 31, 2023 · ddbb003 · ddbb003
1 parent e238512
commit ddbb003
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 33 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
@@ -198,7 +198,7 @@ private static void verifyGeneratingActionKeys(
         && (!allowSharedAction
             || !Actions.canBeSharedLogForPotentialFalsePositives(
                 actionKeyContext, actions.get(actionIndex), actions.get(otherIndex)))) {
-      throw new ActionConflictException(
+      throw ActionConflictException.create(
           actionKeyContext, output, actions.get(actionIndex), actions.get(otherIndex));
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
@@ -57,7 +57,7 @@ public void registerAction(ActionAnalysisMetadata action)
         // advice on possible known-common reasons for conflicts.
         // e.g. If only config diffs where one is missing TestOptions: --trim_test_configuration
         //   or, if the --platforms differ but have same shortname
-        throw new ActionConflictException(actionKeyContext, artifact, previousAction, action);
+        throw ActionConflictException.create(actionKeyContext, artifact, previousAction, action);
       }
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/actions/MutableActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MutableActionGraph.java
@@ -64,34 +64,41 @@ void registerAction(ActionAnalysisMetadata action)
   final class ActionConflictException extends Exception implements SaneAnalysisException {
 
     private final Artifact artifact;
-    private final String suffix;
 
     private static final int MAX_DIFF_ARTIFACTS_TO_REPORT = 5;
 
-    public ActionConflictException(
+    public static ActionConflictException create(
         ActionKeyContext actionKeyContext,
         Artifact artifact,
         ActionAnalysisMetadata previousAction,
         ActionAnalysisMetadata attemptedAction) {
-      super(
-          String.format(
-              "for %s, previous action: %s, attempted action: %s",
-              artifact.prettyPrint(), previousAction.prettyPrint(), attemptedAction.prettyPrint()));
+      return new ActionConflictException(
+          artifact,
+          createDetailedMessage(artifact, actionKeyContext, attemptedAction, previousAction));
+    }
+
+    private ActionConflictException(Artifact artifact, String message) {
+      super(message);
       this.artifact = artifact;
-      this.suffix = debugSuffix(actionKeyContext, attemptedAction, previousAction);
     }
 
     public Artifact getArtifact() {
       return artifact;
     }
 
+    private static String createDetailedMessage(
+        Artifact artifact,
+        ActionKeyContext actionKeyContext,
+        ActionAnalysisMetadata a,
+        ActionAnalysisMetadata b) {
+      return "file '"
+          + artifact.prettyPrint()
+          + "' is generated by these conflicting actions:\n"
+          + debugSuffix(actionKeyContext, a, b);
+    }
+
     public void reportTo(EventHandler eventListener) {
-      String msg =
-          "file '"
-              + artifact.prettyPrint()
-              + "' is generated by these conflicting actions:\n"
-              + suffix;
-      eventListener.handle(Event.error(msg));
+      eventListener.handle(Event.error(this.getMessage()));
     }
 
     @Override

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -1127,9 +1127,7 @@ public void topLevelConflictDetected() throws Exception {
                     "//foo:foo"));
     assertThat(exception)
         .hasMessageThat()
-        .containsMatch(
-            "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect .',"
-                + " attempted action: action 'Action for aspect .'");
+        .containsMatch("ConflictException: file 'foo/aspect.out'");
     MoreAsserts.assertContainsEvent(
         eventCollector,
         Pattern.compile(
@@ -1151,20 +1149,19 @@ public void topLevelConflictDetected() throws Exception {
     scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "2"));
     // Expect errors.
     reporter.removeHandler(failFastHandler);
-    exception =
-        assertThrows(
-            ViewCreationFailedException.class,
-            () ->
-                update(
-                    new EventBus(),
-                    defaultFlags(),
-                    ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"),
-                    "//foo:foo"));
-    assertThat(exception)
-        .hasMessageThat()
-        .containsMatch(
-            "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect .',"
-                + " attempted action: action 'Action for aspect .'");
+    assertThrows(
+        ViewCreationFailedException.class,
+        () ->
+            update(
+                new EventBus(),
+                defaultFlags(),
+                ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"),
+                "//foo:foo"));
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        Pattern.compile(
+            "Aspects: \\[//foo:aspect.bzl%aspect[12]], \\[//foo:aspect.bzl%aspect[12]]"),
+        EventKind.ERROR);
   }
 
   @Test
@@ -1200,7 +1197,9 @@ public void conflictBetweenTargetAndAspect() throws Exception {
     reporter.removeHandler(failFastHandler);
     ViewCreationFailedException exception =
         assertThrows(ViewCreationFailedException.class, () -> update("//foo:foo"));
-    assertThat(exception).hasMessageThat().containsMatch("ConflictException: for foo/conflict.out");
+    assertThat(exception)
+        .hasMessageThat()
+        .containsMatch("ConflictException: file 'foo/conflict.out'");
     MoreAsserts.assertContainsEvent(
         eventCollector,
         Pattern.compile(