diff options
| -rw-r--r-- | src/main/java/com/google/devtools/build/lib/actions/MutableActionGraph.java | 81 | ||||
| -rw-r--r-- | src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java | 55 |
2 files changed, 112 insertions, 24 deletions
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 c72e02e51d..d5da897c43 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 @@ -15,12 +15,12 @@ package com.google.devtools.build.lib.actions; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.common.collect.Sets.SetView; 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.util.StringUtil; import java.util.Set; @@ -67,11 +67,17 @@ public interface MutableActionGraph extends ActionGraph { private final Artifact artifact; private final Action previousAction; private final Action attemptedAction; + + private static final int MAX_DIFF_ARTIFACTS_TO_REPORT = 5; public ActionConflictException(Artifact artifact, Action previousAction, Action attemptedAction) { - super(String.format("for %s, previous action: %s, attempted action: %s", - artifact, previousAction, attemptedAction)); + super( + String.format( + "for %s, previous action: %s, attempted action: %s", + artifact.prettyPrint(), + previousAction.prettyPrint(), + attemptedAction.prettyPrint())); this.artifact = artifact; this.previousAction = previousAction; this.attemptedAction = attemptedAction; @@ -82,13 +88,16 @@ public interface MutableActionGraph extends ActionGraph { } public void reportTo(EventHandler eventListener) { - String msg = "file '" + artifact.prettyPrint() - + "' is generated by these conflicting actions:\n" + - suffix(attemptedAction, previousAction); + String msg = + "file '" + + artifact.prettyPrint() + + "' is generated by these conflicting actions:\n" + + suffix(attemptedAction, previousAction); eventListener.handle(Event.error(msg)); } - private void addStringDetail(StringBuilder sb, String key, String valueA, String valueB) { + private static void addStringDetail( + StringBuilder sb, String key, String valueA, String valueB) { valueA = valueA != null ? valueA : "(null)"; valueB = valueB != null ? valueB : "(null)"; @@ -99,8 +108,8 @@ public interface MutableActionGraph extends ActionGraph { sb.append("\n"); } - private void addListDetail(StringBuilder sb, String key, - Iterable<Artifact> valueA, Iterable<Artifact> valueB) { + private static void addListDetail( + StringBuilder sb, String key, Iterable<Artifact> valueA, Iterable<Artifact> valueB) { Set<Artifact> setA = ImmutableSet.copyOf(valueA); Set<Artifact> setB = ImmutableSet.copyOf(valueB); SetView<Artifact> diffA = Sets.difference(setA, setB); @@ -108,21 +117,31 @@ public interface MutableActionGraph extends ActionGraph { sb.append(key).append(": "); if (diffA.isEmpty() && diffB.isEmpty()) { - sb.append("are equal"); + sb.append("are equal\n"); } else { - if (!diffA.isEmpty() && !diffB.isEmpty()) { - sb.append("attempted action contains artifacts not in previous action and " - + "previous action contains artifacts not in attempted action: " - + diffA + ", " + diffB); - } else if (!diffA.isEmpty()) { - sb.append("attempted action contains artifacts not in previous action: "); - sb.append(StringUtil.joinEnglishList(diffA, "and")); - } else if (!diffB.isEmpty()) { - sb.append("previous action contains artifacts not in attempted action: "); - sb.append(StringUtil.joinEnglishList(diffB, "and")); + if (!diffA.isEmpty()) { + sb.append( + "Attempted action contains artifacts not in previous action (first " + + MAX_DIFF_ARTIFACTS_TO_REPORT + + "): \n"); + prettyPrintArtifactDiffs(sb, diffA); + } + + if (!diffB.isEmpty()) { + sb.append( + "Previous action contains artifacts not in attempted action (first " + + MAX_DIFF_ARTIFACTS_TO_REPORT + + "): \n"); + prettyPrintArtifactDiffs(sb, diffB); } } - sb.append("\n"); + } + + /** Pretty print action diffs (at most {@code MAX_DIFF_ARTIFACTS_TO_REPORT} lines). */ + private static void prettyPrintArtifactDiffs(StringBuilder sb, SetView<Artifact> diff) { + for (Artifact artifact : Iterables.limit(diff, MAX_DIFF_ARTIFACTS_TO_REPORT)) { + sb.append("\t" + artifact.prettyPrint() + "\n"); + } } // See also Actions.canBeShared() @@ -144,9 +163,23 @@ public interface MutableActionGraph extends ActionGraph { bNull ? null : bOwner.getConfigurationChecksum()); addStringDetail(sb, "Mnemonic", a.getMnemonic(), b.getMnemonic()); addStringDetail(sb, "Progress message", a.getProgressMessage(), b.getProgressMessage()); - - addListDetail(sb, "MandatoryInputs", a.getMandatoryInputs(), b.getMandatoryInputs()); - addListDetail(sb, "Outputs", a.getOutputs(), b.getOutputs()); + addStringDetail( + sb, + "PrimaryInput", + a.getPrimaryInput() == null ? null : a.getPrimaryInput().toString(), + b.getPrimaryInput() == null ? null : b.getPrimaryInput().toString()); + addStringDetail( + sb, "PrimaryOutput", a.getPrimaryOutput().toString(), b.getPrimaryOutput().toString()); + + // Only add list details if the primary input of A matches the input of B. Otherwise + // the above information is enough and list diff detail is not needed. + if ((a.getPrimaryInput() == null && b.getPrimaryInput() == null) + || (a.getPrimaryInput() != null + && b.getPrimaryInput() != null + && a.getPrimaryInput().toString().equals(b.getPrimaryInput().toString()))) { + addListDetail(sb, "MandatoryInputs", a.getMandatoryInputs(), b.getMandatoryInputs()); + addListDetail(sb, "Outputs", a.getOutputs(), b.getOutputs()); + } return sb.toString(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java index 84049bdc67..857a01f23e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java @@ -24,6 +24,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.analysis.util.AnalysisCachingTestBase; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider; import com.google.devtools.build.lib.testutil.Suite; import com.google.devtools.build.lib.testutil.TestSpec; @@ -33,6 +34,8 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * Analysis caching tests. @@ -205,6 +208,58 @@ public class AnalysisCachingTest extends AnalysisCachingTestBase { "//conflict:x", "//conflict:_objs/x/conflict/foo.pic.o"); assertNoEvents(); } + + /** + * For two conflicting actions whose primary inputs are different, no list diff detail should be + * part of the output. + */ + @Test + public void testConflictingArtifactsErrorWithNoListDetail() throws Exception { + scratch.file( + "conflict/BUILD", + "cc_library(name='x', srcs=['foo.cc'])", + "cc_binary(name='_objs/x/conflict/foo.pic.o', srcs=['bar.cc'])"); + reporter.removeHandler(failFastHandler); // expect errors + update( + defaultFlags().with(Flag.KEEP_GOING), + "//conflict:x", + "//conflict:_objs/x/conflict/foo.pic.o"); + + assertContainsEvent("file 'conflict/_objs/x/conflict/foo.pic.o' " + CONFLICT_MSG); + assertDoesNotContainEvent("MandatoryInputs"); + assertDoesNotContainEvent("Outputs"); + } + + /** + * For two conflicted actions whose primary inputs are the same, list diff (max 5) should be part + * of the output. + */ + @Test + public void testConflictingArtifactsWithListDetail() throws Exception { + scratch.file( + "conflict/BUILD", + "cc_library(name='x', srcs=['foo1.cc', 'foo2.cc', 'foo3.cc', 'foo4.cc', 'foo5.cc'" + + ", 'foo6.cc'])", + "genrule(name = 'foo', outs=['_objs/x/conflict/foo1.pic.o'], srcs=['foo1.cc', 'foo2.cc', " + + "'foo3.cc', 'foo4.cc', 'foo5.cc', 'foo6.cc'], cmd='', output_to_bindir=1)"); + reporter.removeHandler(failFastHandler); // expect errors + update(defaultFlags().with(Flag.KEEP_GOING), "//conflict:x", "//conflict:foo"); + + Event event = + assertContainsEvent("file 'conflict/_objs/x/conflict/foo1.pic.o' " + CONFLICT_MSG); + assertContainsEvent("MandatoryInputs"); + assertContainsEvent("Outputs"); + + // Validate that maximum of 5 artifacts in MandatoryInputs are part of output. + Pattern pattern = Pattern.compile("\tconflict\\/foo[1-6].cc"); + Matcher matcher = pattern.matcher(event.getMessage()); + int matchCount = 0; + while (matcher.find()) { + matchCount++; + } + + assertEquals(5, matchCount); + } /** * The current action conflict detection code will only mark one of the targets as having an |