From 0175ce3630f15262172731e00e8413c534ed6a62 Mon Sep 17 00:00:00 2001 From: janakr Date: Mon, 26 Feb 2018 15:54:57 -0800 Subject: Fail gracefully on conflicting actions generated by an aspect. These can come from Skylark, so we shouldn't crash. As a safety measure, subclasses of ActionLookupValue are now responsible for detecting action conflicts themselves. PiperOrigin-RevId: 187095271 --- .../devtools/build/lib/actions/ActionLookupValueTest.java | 9 ++++++--- .../lib/skyframe/ActionTemplateExpansionFunctionTest.java | 9 +++------ .../devtools/build/lib/skyframe/ArtifactFunctionTest.java | 11 +++++++---- .../build/lib/skyframe/TimestampBuilderTestCase.java | 15 ++++++++++++--- .../lib/skyframe/ToolchainResolutionFunctionTest.java | 4 ++-- .../build/lib/skyframe/TreeArtifactBuildTest.java | 9 ++++++++- .../build/lib/skyframe/TreeArtifactMetadataTest.java | 11 +++++++---- 7 files changed, 45 insertions(+), 23 deletions(-) (limited to 'src/test/java') diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java index e2ddd4f55f..f6c5961941 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java @@ -20,6 +20,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -47,7 +48,7 @@ public class ActionLookupValueTest { Artifact artifact = mock(Artifact.class); when(action.getOutputs()).thenReturn(ImmutableSet.of(artifact)); when(action.canRemoveAfterExecution()).thenReturn(true); - ActionLookupValue underTest = new ActionLookupValue(actionKeyContext, action, false); + ActionLookupValue underTest = new ActionLookupValue(action, false); assertThat(underTest.getGeneratingActionIndex(artifact)).isEqualTo(0); assertThat(underTest.getAction(0)).isSameAs(action); underTest.actionEvaluated(0, action); @@ -55,7 +56,7 @@ public class ActionLookupValueTest { } @Test - public void testActionNotPresentAfterEvaluation() { + public void testActionNotPresentAfterEvaluation() throws ActionConflictException { Path execRoot = fs.getPath("/execroot"); Path outputRootPath = execRoot.getRelative("blaze-out"); ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, outputRootPath); @@ -69,7 +70,9 @@ public class ActionLookupValueTest { when(persistentAction.canRemoveAfterExecution()).thenReturn(false); ActionLookupValue underTest = new ActionLookupValue( - actionKeyContext, ImmutableList.of(normalAction, persistentAction), true); + Actions.filterSharedActionsAndThrowActionConflict( + actionKeyContext, ImmutableList.of(normalAction, persistentAction)), + true); assertThat(underTest.getGeneratingActionIndex(normalArtifact)).isEqualTo(0); assertThat(underTest.getAction(0)).isSameAs(normalAction); assertThat(underTest.getGeneratingActionIndex(persistentOutput)).isEqualTo(1); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index dba65ebbdc..ba1a3c2e3b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java @@ -196,9 +196,7 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas ConfiguredTargetKey.of(Label.parseAbsoluteUnchecked("//foo:foo"), null); private List evaluate(SpawnActionTemplate spawnActionTemplate) throws Exception { - ConfiguredTargetValue ctValue = - createConfiguredTargetValue( - spawnActionTemplate.getOutputTreeArtifact(), spawnActionTemplate); + ConfiguredTargetValue ctValue = createConfiguredTargetValue(spawnActionTemplate); differencer.inject(CTKEY, ctValue); ActionTemplateExpansionKey templateKey = ActionTemplateExpansionValue.key(CTKEY, 0); @@ -220,11 +218,10 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas } private static ConfiguredTargetValue createConfiguredTargetValue( - Artifact artifact, ActionTemplate actionTemplate) { + ActionTemplate actionTemplate) { return new ConfiguredTargetValue( Mockito.mock(ConfiguredTarget.class), - new Actions.GeneratingActions( - ImmutableList.of(actionTemplate), ImmutableMap.of(artifact, 0)), + Actions.GeneratingActions.fromSingleAction(actionTemplate), NestedSetBuilder.stableOrder().build(), /*removeActionsAfterEvaluation=*/ false); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index 0a8c460803..8a4f3af2e1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -22,17 +22,18 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MissingInputFileException; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.TestAction.DummyAction; import com.google.devtools.build.lib.events.NullEventHandler; @@ -318,18 +319,20 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase { return result.get(key); } - private void setGeneratingActions() throws InterruptedException { + private void setGeneratingActions() throws InterruptedException, ActionConflictException { if (evaluator.getExistingValue(ALL_OWNER) == null) { differencer.inject( ImmutableMap.of( ALL_OWNER, new ActionLookupValue( - actionKeyContext, ImmutableList.copyOf(actions), false))); + Actions.filterSharedActionsAndThrowActionConflict( + actionKeyContext, ImmutableList.copyOf(actions)), + false))); } } private EvaluationResult evaluate(SkyKey... keys) - throws InterruptedException { + throws InterruptedException, ActionConflictException { setGeneratingActions(); return driver.evaluate( Arrays.asList(keys), diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 278f1d01af..d2aca56fe9 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -38,10 +38,12 @@ import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.ActionResult; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.Executor; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.ResourceManager; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.TestExecException; @@ -233,12 +235,15 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); return new Builder() { - private void setGeneratingActions() { + private void setGeneratingActions() throws ActionConflictException { if (evaluator.getExistingValue(ACTION_LOOKUP_KEY) == null) { differencer.inject( ImmutableMap.of( ACTION_LOOKUP_KEY, - new ActionLookupValue(actionKeyContext, ImmutableList.copyOf(actions), false))); + new ActionLookupValue( + Actions.filterSharedActionsAndThrowActionConflict( + actionKeyContext, ImmutableList.copyOf(actions)), + false))); } } @@ -274,7 +279,11 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { keys.add(ArtifactSkyKey.key(artifact, true)); } - setGeneratingActions(); + try { + setGeneratingActions(); + } catch (ActionConflictException e) { + throw new IllegalStateException(e); + } EvaluationResult result = driver.evaluate(keys, keepGoing, threadCount, reporter); if (result.hasError()) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java index 4f8b3bc4a0..0194819122 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java @@ -22,7 +22,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.testing.EqualsTester; -import com.google.devtools.build.lib.actions.Actions; +import com.google.devtools.build.lib.actions.Actions.GeneratingActions; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.cmdline.Label; @@ -48,7 +48,7 @@ public class ToolchainResolutionFunctionTest extends ToolchainTestCase { ConfiguredTarget configuredTarget) { return new ConfiguredTargetValue( configuredTarget, - new Actions.GeneratingActions(ImmutableList.of(), ImmutableMap.of()), + GeneratingActions.EMPTY, NestedSetBuilder.emptySet(Order.STABLE_ORDER), /*removeActionsAfterEvaluation=*/ false); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index 411fef60fb..b15782c030 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -34,12 +34,14 @@ import com.google.devtools.build.lib.actions.ActionInputFileCache; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionResult; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.TestAction; @@ -1232,7 +1234,12 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase { @Override public SkyValue compute(SkyKey skyKey, Environment env) { - return new ActionTemplateExpansionValue(actionKeyContext, actions, false); + try { + return new ActionTemplateExpansionValue( + Actions.filterSharedActionsAndThrowActionConflict(actionKeyContext, actions), false); + } catch (ActionConflictException e) { + throw new IllegalStateException(e); + } } @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index aaadc2295a..7a804e44d2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -24,16 +24,17 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MissingInputFileException; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.Metadata; import com.google.devtools.build.lib.actions.util.TestAction.DummyAction; @@ -222,18 +223,20 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase { return result.get(key); } - private void setGeneratingActions() throws InterruptedException { + private void setGeneratingActions() throws InterruptedException, ActionConflictException { if (evaluator.getExistingValue(ALL_OWNER) == null) { differencer.inject( ImmutableMap.of( ALL_OWNER, new ActionLookupValue( - actionKeyContext, ImmutableList.copyOf(actions), false))); + Actions.filterSharedActionsAndThrowActionConflict( + actionKeyContext, ImmutableList.copyOf(actions)), + false))); } } private EvaluationResult evaluate(SkyKey... keys) - throws InterruptedException { + throws InterruptedException, ActionConflictException { setGeneratingActions(); return driver.evaluate( Arrays.asList(keys), /*keepGoing=*/ -- cgit v1.2.3