From 0f9f470c415edf1ecc3e6ca6a0253a8aa86f7e45 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Mon, 28 Jan 2019 14:27:35 +0100 Subject: [PATCH] Add ability for a Spawn to declare a required local output Some native actions run both remotely and locally. For example, the CppCompileAction runs compilation remotely but does .d file pruning locally. This change adds the necessary infrastructure for an action to tell a Spawn which of its outputs it needs on the local filesytem. Progress towards #6862 --- .../lib/actions/ActionExecutionContext.java | 43 +++++++++++++++++++ .../build/lib/exec/AbstractSpawnStrategy.java | 7 +++ .../devtools/build/lib/exec/SpawnRunner.java | 18 +++++++- .../lib/skyframe/SkyframeActionExecutor.java | 1 + .../actions/ExecutableSymlinkActionTest.java | 2 + .../lib/actions/util/ActionsTestUtil.java | 2 + .../actions/FileWriteActionTestCase.java | 2 + .../actions/ParamFileWriteActionTest.java | 1 + .../analysis/actions/SymlinkActionTest.java | 2 + .../actions/TemplateExpansionActionTest.java | 1 + .../lib/analysis/util/BuildViewTestCase.java | 1 + .../lib/exec/StandaloneTestStrategyTest.java | 1 + .../rules/cpp/CreateIncSymlinkActionTest.java | 2 + .../lib/rules/cpp/LtoBackendActionTest.java | 2 + .../rules/objc/BazelJ2ObjcLibraryTest.java | 2 + .../StandaloneSpawnStrategyTest.java | 1 + 16 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index fe4bf4221eb0e1..582fba32335edf 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -32,6 +32,7 @@ import com.google.devtools.common.options.OptionsProvider; import java.io.Closeable; import java.io.IOException; +import java.util.Collection; import java.util.Map; import javax.annotation.Nullable; @@ -72,6 +73,8 @@ public enum ShowSubcommands { private final ArtifactPathResolver pathResolver; + private final ImmutableList requiredLocalOutputs; + private ActionExecutionContext( Executor executor, MetadataProvider actionInputFileCache, @@ -82,6 +85,7 @@ private ActionExecutionContext( ExtendedEventHandler eventHandler, Map clientEnv, ImmutableMap> topLevelFilesets, + ImmutableList requiredLocalOutputs, @Nullable ArtifactExpander artifactExpander, @Nullable Environment env, @Nullable FileSystem actionFileSystem, @@ -102,6 +106,7 @@ private ActionExecutionContext( this.pathResolver = ArtifactPathResolver.createPathResolver(actionFileSystem, // executor is only ever null in testing. executor == null ? null : executor.getExecRoot()); + this.requiredLocalOutputs = Preconditions.checkNotNull(requiredLocalOutputs); } public ActionExecutionContext( @@ -114,6 +119,7 @@ public ActionExecutionContext( ExtendedEventHandler eventHandler, Map clientEnv, ImmutableMap> topLevelFilesets, + ImmutableList requiredLocalOutputs, ArtifactExpander artifactExpander, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -127,6 +133,7 @@ public ActionExecutionContext( eventHandler, clientEnv, topLevelFilesets, + requiredLocalOutputs, artifactExpander, /*env=*/ null, actionFileSystem, @@ -154,6 +161,7 @@ public static ActionExecutionContext forInputDiscovery( eventHandler, clientEnv, ImmutableMap.of(), + /*requiredLocalOutputs=*/ ImmutableList.of(), /*artifactExpander=*/ null, env, actionFileSystem, @@ -307,6 +315,18 @@ public ActionKeyContext getActionKeyContext() { return actionKeyContext; } + /** + * Returns the collection of files that this command must write and make available via + * the local {@link com.google.devtools.build.lib.vfs.FileSystem}. The returned output + * artifacts are a subset of the action's output artifacts. + * + *

This is for use with remote execution, where as an optimization we don't want to + * download all output files. + */ + public Collection getRequiredLocalOutputs() { + return requiredLocalOutputs; + } + @Override public void close() throws IOException { fileOutErr.close(); @@ -327,6 +347,29 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) { eventHandler, clientEnv, topLevelFilesets, + requiredLocalOutputs, + artifactExpander, + env, + actionFileSystem, + skyframeDepsResult); + } + + /** + * Allows us to create a new context that provides a list of output artifacts that need to + * be staged on the local filesystem. + */ + public ActionExecutionContext withRequiredLocalOutputs(Collection requiredLocalOutputs) { + return new ActionExecutionContext( + executor, + actionInputFileCache, + actionInputPrefetcher, + actionKeyContext, + metadataHandler, + fileOutErr, + eventHandler, + clientEnv, + topLevelFilesets, + ImmutableList.copyOf(requiredLocalOutputs), artifactExpander, env, actionFileSystem, diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index e95413b00f4e58..a65398eb0c5110 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionStatusMessage; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.EnvironmentalExecException; @@ -43,6 +44,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; +import java.util.Collection; import java.util.List; import java.util.SortedMap; import java.util.concurrent.atomic.AtomicInteger; @@ -271,5 +273,10 @@ public void report(ProgressStatus state, String name) { break; } } + + @Override + public Collection getRequiredLocalOutputs() { + return actionExecutionContext.getRequiredLocalOutputs(); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java index 0d70563e407ebc..3458bafdb58367 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java @@ -13,7 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.exec; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ExecException; @@ -25,6 +27,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; +import java.util.Collection; import java.util.SortedMap; /** @@ -129,6 +132,7 @@ public enum ProgressStatus { * by different threads, so they MUST not call any shared non-thread-safe objects. */ interface SpawnExecutionContext { + /** * Returns a unique id for this spawn, to be used for logging. Note that a single spawn may be * passed to multiple {@link SpawnRunner} implementations, so any log entries should also @@ -199,6 +203,18 @@ SortedMap getInputMapping(boolean expandTreeArtifacts /** Reports a progress update to the Spawn strategy. */ void report(ProgressStatus state, String name); + + /** + * Returns the collection of files that this command must write and make available via + * the local {@link com.google.devtools.build.lib.vfs.FileSystem}. The returned output + * artifacts are a subset of the action's output artifacts. + * + *

This is for use with remote execution, where as an optimization we don't want to + * download all output files. + */ + default Collection getRequiredLocalOutputs() { + return ImmutableList.of(); + } } /** @@ -234,4 +250,4 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context) /* Name of the SpawnRunner. */ String getName(); -} +} \ No newline at end of file diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 1e8f9ba2362cb8..dec1d0516a67e8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -601,6 +601,7 @@ public ActionExecutionContext getContext( : progressSuppressingEventHandler, clientEnv, topLevelFilesets, + ImmutableList.of(), new ArtifactExpanderImpl(expandedInputs, expandedFilesets), actionFileSystem, skyframeDepsResult); diff --git a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java index 9fa6ae3ceb647d..3deb3f6dedd388 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java @@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; import static org.junit.Assert.fail; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; @@ -66,6 +67,7 @@ private ActionExecutionContext createContext() { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + /*requiredLocalOutputs=*/ ImmutableList.of(), null, null, null); diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 7054d57b0ee960..d6778ad92a1cf0 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -147,6 +147,7 @@ public static ActionExecutionContext createContext( executor != null ? executor.getEventHandler() : null, ImmutableMap.copyOf(clientEnv), ImmutableMap.of(), + ImmutableList.of(), actionGraph == null ? createDummyArtifactExpander() : ActionInputHelper.actionGraphArtifactExpander(actionGraph), @@ -187,6 +188,7 @@ public static ActionExecutionContext createContext(ExtendedEventHandler eventHan eventHandler, ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), createDummyArtifactExpander(), /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java index 32f6fa10e0dddb..6687b423658e38 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; @@ -71,6 +72,7 @@ public final void createExecutorAndContext() throws Exception { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), null, null, null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java index 7625475b30927a..20eb30c8d6520a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java @@ -203,6 +203,7 @@ public void expand(Artifact artifact, Collection output) { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), artifactExpander, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java index 20681509707760..c639eb6b965e60 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; @@ -89,6 +90,7 @@ public void testSymlink() throws Exception { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), null, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null)); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java index 3495ad15123eae..ab889d287a91bb 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java @@ -192,6 +192,7 @@ private ActionExecutionContext createContext(Executor executor) { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), null, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 42869c94f79b5d..cbee0a1f61532b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -2231,6 +2231,7 @@ public ActionExecutionContext build() { reporter, clientEnv, ImmutableMap.of(), + ImmutableList.of(), artifactExpander, /*actionFileSystem=*/ null, /*skyframeDepsResult*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java index b499f26a83ccc3..ee192f0d7ec60a 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java @@ -96,6 +96,7 @@ public FakeActionExecutionContext( /*eventHandler=*/ null, /* clientEnv= */ ImmutableMap.of(), /* topLevelFilesets= */ ImmutableMap.of(), + /* requiredLocalOutputs= */ ImmutableList.of(), /* artifactExpander= */ null, /* actionFileSystem= */ null, /* skyframeDepsResult= */ null); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java index 04768d3ec05e79..37814ab660e8e6 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionKeyContext; @@ -125,6 +126,7 @@ private ActionExecutionContext makeDummyContext() { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), null, null, null); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java index eddcc43d12d4b0..0987a7f5e8d718 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java @@ -15,6 +15,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.AbstractAction; @@ -90,6 +91,7 @@ public final void createExecutorAndContext() throws Exception { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), null, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java index 34a29896ce7d9d..bc615ff3bece8d 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java @@ -794,6 +794,7 @@ public void testJ2ObjCCustomModuleMap() throws Exception { null, ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), DUMMY_ARTIFACT_EXPANDER, null, null); @@ -847,6 +848,7 @@ public void testModuleMapFromGenJarTreeArtifact() throws Exception { null, ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), DUMMY_ARTIFACT_EXPANDER, null, null); diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java index 8272493e94337c..1fa251eb1cd5db 100644 --- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java @@ -195,6 +195,7 @@ private ActionExecutionContext createContext() { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), SIMPLE_ARTIFACT_EXPANDER, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null);