Clean-up for list-based strategy selection
Removed unused code after list-based execution strategy was enabled by default
Closes #8970
Closes #9332.
PiperOrigin-RevId: 271343455
diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java
index 7e04b67..f5dbde2 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java
@@ -59,28 +59,17 @@
public static final ExecutionOptions DEFAULTS = Options.getDefaults(ExecutionOptions.class);
@Option(
- name = "incompatible_list_based_execution_strategy_selection",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
- effectTags = {OptionEffectTag.EXECUTION},
- metadataTags = {
- OptionMetadataTag.INCOMPATIBLE_CHANGE,
- OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
- },
- help = "See https://github.com/bazelbuild/bazel/issues/7480")
- public boolean incompatibleListBasedExecutionStrategySelection;
-
- @Option(
name = "spawn_strategy",
defaultValue = "",
converter = CommaSeparatedNonEmptyOptionListConverter.class,
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
- "Specify how spawn actions are executed by default. "
- + "'standalone' means run all of them locally without any kind of sandboxing. "
- + "'sandboxed' means to run them in a sandboxed environment with limited privileges "
- + "(details depend on platform support).")
+ "Specify how spawn actions are executed by default. Accepts a comma-separated list of"
+ + " strategies from highest to lowest priority. For each action Bazel picks the"
+ + " strategy with the highest priority that can execute the action. The default"
+ + " value is \"remote,worker,sandboxed,local\".See"
+ + " https://blog.bazel.build/2019/06/19/list-strategy.html for details.")
public List<String> spawnStrategy;
@Option(
@@ -103,9 +92,11 @@
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
- "Specify how to distribute compilation of other spawn actions. "
- + "Example: 'Javac=local' means to spawn Java compilation locally. "
- + "'JavaIjar=sandboxed' means to spawn Java Ijar actions in a sandbox. ")
+ "Specify how to distribute compilation of other spawn actions. Accepts a comma-separated"
+ + " list of strategies from highest to lowest priority. For each action Bazel picks"
+ + " the strategy with the highest priority that can execute the action. The default"
+ + " value is \"remote,worker,sandboxed,local\".See"
+ + " https://blog.bazel.build/2019/06/19/list-strategy.html for details.")
public List<Map.Entry<String, List<String>>> strategy;
@Option(
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 6412122..181b343 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -125,7 +125,7 @@
String buildRequestId,
String commandId,
GrpcRemoteCache remoteCache,
- @Nullable GrpcRemoteExecutor remoteExecutor,
+ GrpcRemoteExecutor remoteExecutor,
@Nullable RemoteRetrier retrier,
DigestUtil digestUtil,
Path logDir,
@@ -135,7 +135,7 @@
this.executionOptions = executionOptions;
this.fallbackRunner = fallbackRunner;
this.remoteCache = Preconditions.checkNotNull(remoteCache, "remoteCache");
- this.remoteExecutor = remoteExecutor;
+ this.remoteExecutor = Preconditions.checkNotNull(remoteExecutor, "remoteExecutor");
this.verboseFailures = verboseFailures;
this.cmdlineReporter = cmdlineReporter;
this.buildRequestId = buildRequestId;
@@ -178,11 +178,8 @@
commandHash, merkleTree.getRootDigest(), context.getTimeout(), spawnCacheableRemotely);
ActionKey actionKey = digestUtil.computeActionKey(action);
- if (!Spawns.mayBeExecutedRemotely(spawn)) {
- return execLocallyAndUpload(
- spawn, context, inputMap, actionKey, action, command, uploadLocalResults);
- }
-
+ Preconditions.checkArgument(
+ Spawns.mayBeExecutedRemotely(spawn), "Spawn can't be executed remotely. This is a bug.");
// Look up action cache, and reuse the action output if it is found.
Context withMetadata =
TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey);
@@ -217,12 +214,6 @@
spawn, context, inputMap, actionKey, action, command, uploadLocalResults, e);
}
- if (remoteExecutor == null) {
- // Remote execution is disabled and so execute the spawn on the local machine.
- return execLocallyAndUpload(
- spawn, context, inputMap, actionKey, action, command, uploadLocalResults);
- }
-
ExecuteRequest.Builder requestBuilder =
ExecuteRequest.newBuilder()
.setInstanceName(remoteOptions.remoteInstanceName)
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index f2ca67e..cf52c57 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -253,46 +253,16 @@
}
@Test
- public void noRemoteExecExecutesLocallyButUsesCache() throws Exception {
- // Test that if the NO_REMOTE_EXEC execution requirement is specified, the spawn is executed
- // locally, but its result is still uploaded to the remote cache.
+ public void failedLocalActionShouldNotBeUploaded() throws Exception {
+ // Test that the outputs of a locally executed action that failed are not uploaded.
- remoteOptions.remoteAcceptCached = true;
+ remoteOptions.remoteLocalFallback = true;
remoteOptions.remoteUploadLocalResults = true;
RemoteSpawnRunner runner = spy(newSpawnRunner());
- SpawnResult res =
- new SpawnResult.Builder()
- .setStatus(Status.SUCCESS)
- .setExitCode(0)
- .setRunnerName("test")
- .build();
- when(localRunner.exec(any(Spawn.class), any(SpawnExecutionContext.class))).thenReturn(res);
-
- Spawn spawn =
- simpleSpawnWithExecutionInfo(ImmutableMap.of(ExecutionRequirements.NO_REMOTE_EXEC, ""));
- SpawnExecutionContext policy =
- new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, outErr);
-
- SpawnResult result = runner.exec(spawn, policy);
-
- assertThat(result.exitCode()).isEqualTo(0);
- assertThat(result.status()).isEqualTo(Status.SUCCESS);
- verify(localRunner).exec(eq(spawn), eq(policy));
- verify(runner)
- .execLocallyAndUpload(
- eq(spawn), eq(policy), any(), any(), any(), any(), /* uploadLocalResults= */ eq(true));
- verify(cache).upload(any(), any(), any(), any(), any(), any());
- }
-
- @Test
- public void failedLocalActionShouldNotBeUploaded() throws Exception {
- // Test that the outputs of a locally executed action that failed are not uploaded.
-
- remoteOptions.remoteUploadLocalResults = true;
-
- RemoteSpawnRunner runner = spy(newSpawnRunnerWithoutExecutor());
+ // Throw an IOException to trigger the local fallback.
+ when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(IOException.class);
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy =
@@ -323,10 +293,15 @@
// Test that bazel treats failed cache action as a cache miss and attempts to execute action
// locally
+ remoteOptions.remoteLocalFallback = true;
+ remoteOptions.remoteUploadLocalResults = true;
+
ActionResult failedAction = ActionResult.newBuilder().setExitCode(1).build();
when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(failedAction);
- RemoteSpawnRunner runner = spy(newSpawnRunnerWithoutExecutor());
+ RemoteSpawnRunner runner = spy(newSpawnRunner());
+ // Throw an IOException to trigger the local fallback.
+ when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(IOException.class);
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy =
@@ -393,7 +368,9 @@
StoredEventHandler eventHandler = new StoredEventHandler();
reporter.addHandler(eventHandler);
- RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor(reporter);
+ RemoteSpawnRunner runner = newSpawnRunner(reporter);
+ // Trigger local fallback
+ when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy =
@@ -428,12 +405,14 @@
@Test
public void noRemoteExecutorFallbackFails() throws Exception {
- // Errors from the fallback runner should be propogated out of the remote runner.
+ // Errors from the fallback runner should be propagated out of the remote runner.
remoteOptions.remoteUploadLocalResults = true;
remoteOptions.remoteLocalFallback = true;
- RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor();
+ RemoteSpawnRunner runner = newSpawnRunner();
+ // Trigger local fallback
+ when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy =
@@ -452,12 +431,14 @@
@Test
public void remoteCacheErrorFallbackFails() throws Exception {
- // Errors from the fallback runner should be propogated out of the remote runner.
+ // Errors from the fallback runner should be propagated out of the remote runner.
remoteOptions.remoteUploadLocalResults = true;
remoteOptions.remoteLocalFallback = true;
- RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor();
+ RemoteSpawnRunner runner = newSpawnRunner();
+ // Trigger local fallback
+ when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy =
@@ -1007,6 +988,11 @@
/* topLevelOutputs= */ ImmutableSet.of());
}
+ private RemoteSpawnRunner newSpawnRunner(Reporter reporter) {
+ return newSpawnRunner(
+ /* verboseFailures= */ false, executor, reporter, /* topLevelOutputs= */ ImmutableSet.of());
+ }
+
private RemoteSpawnRunner newSpawnRunner(ImmutableSet<ActionInput> topLevelOutputs) {
return newSpawnRunner(
/* verboseFailures= */ false, executor, /* reporter= */ null, topLevelOutputs);
@@ -1033,20 +1019,4 @@
logDir,
topLevelOutputs);
}
-
- private RemoteSpawnRunner newSpawnRunnerWithoutExecutor() {
- return newSpawnRunner(
- /* verboseFailures= */ false,
- /* executor= */ null,
- /* reporter= */ null,
- /* topLevelOutputs= */ ImmutableSet.of());
- }
-
- private RemoteSpawnRunner newSpawnRunnerWithoutExecutor(Reporter reporter) {
- return newSpawnRunner(
- /* verboseFailures= */ false,
- /* executor= */ null,
- reporter,
- /* topLevelOutputs= */ ImmutableSet.of());
- }
}
diff --git a/src/test/shell/integration/execution_strategies_test.sh b/src/test/shell/integration/execution_strategies_test.sh
index f45b14c..cb6981b 100755
--- a/src/test/shell/integration/execution_strategies_test.sh
+++ b/src/test/shell/integration/execution_strategies_test.sh
@@ -44,11 +44,6 @@
source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
|| { echo "integration_test_setup.sh not found!" >&2; exit 1; }
-# Tests that a list based strategy selection is enabled by default
-function test_incompatible_flag_flipped() {
- bazel build --spawn_strategy=worker,local --debug_print_action_contexts &> $TEST_log || fail
- expect_not_log "incompatible_list_based_execution_strategy_selection was not enabled"
-}
# Tests that you can set the spawn strategy flags to a list of strategies.
function test_multiple_strategies() {
