From ddbb003f39010cf39b0a44994b55ae1224050b58 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 31 Oct 2023 09:24:50 -0700 Subject: [PATCH] Fix `ActionConflictException` message PiperOrigin-RevId: 578208178 Change-Id: Idbebf35c029f8748f57caab3519a73b10b6b06ca --- .../devtools/build/lib/actions/Actions.java | 2 +- .../lib/actions/MapBasedActionGraph.java | 2 +- .../build/lib/actions/MutableActionGraph.java | 33 ++++++++++------- .../build/lib/analysis/AspectTest.java | 35 +++++++++---------- 4 files changed, 39 insertions(+), 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 index b38a95e4826bf9..a563ad5d221118 100644 --- 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 index 263d88c2a6662e..d61695dae0590c 100644 --- 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 index b56e5402257fb5..d3081b2bf226ef 100644 --- 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 index e6e11c9ea749c6..2a48601449afe1 100644 --- 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(