Repository: bazelbuild/bazel
Issue: clean-up after enabling list based execution strategy
Labels: type: bug
team-Remote-Exec

List based execution strategy was recently introduced at #7480. After it's been enabled, certain scenarios are not possible anymore => code clean-up would be a great thing to do :)

Some places to look at:

- [x]  update docs

- [x]  `incompatible_list_based_execution_strategy_selection` can be deleted with all related logic
- [x]   remote execution fallback to local execution
- [ ]   other strategy related logic

Issue comments:

Pull request review comments:
That text might be a little long for a help text. Consider changing it to 

`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.`
done for both

Patch that resolved the issue:
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 7e04b6768ea9e2..ec83dc053f88fc 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
@@ -58,18 +58,6 @@ public class ExecutionOptions extends OptionsBase {
 
   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 = "",
@@ -78,9 +66,10 @@ public class ExecutionOptions extends OptionsBase {
       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).")
+              + "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(
@@ -104,8 +93,10 @@ public class ExecutionOptions extends OptionsBase {
       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. ")
+              + "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 641212257d4d31..5d596f75a59a29 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 @@ public class RemoteSpawnRunner implements SpawnRunner {
       String buildRequestId,
       String commandId,
       GrpcRemoteCache remoteCache,
-      @Nullable GrpcRemoteExecutor remoteExecutor,
+      GrpcRemoteExecutor remoteExecutor,
       @Nullable RemoteRetrier retrier,
       DigestUtil digestUtil,
       Path logDir,
@@ -178,11 +178,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
             commandHash, merkleTree.getRootDigest(), context.getTimeout(), spawnCacheableRemotely);
     ActionKey actionKey = digestUtil.computeActionKey(action);
 
-    if (!Spawns.mayBeExecutedRemotely(spawn)) {
-      return execLocallyAndUpload(
-          spawn, context, inputMap, actionKey, action, command, uploadLocalResults);
-    }
-
     // Look up action cache, and reuse the action output if it is found.
     Context withMetadata =
         TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey);
@@ -217,12 +212,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
             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)


Test patch:
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 b8beb81f54d6be..29a549aede99fa 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
@@ -252,47 +252,17 @@ public void cachableSpawnsShouldBeCached_localFallback() throws Exception {
     verify(cache).upload(any(), any(), any(), any(), any(), any());
   }
 
-  @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.
-
-    remoteOptions.remoteAcceptCached = 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.remoteLocalFallback = true;
     remoteOptions.remoteUploadLocalResults = true;
 
-    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 =
@@ -323,10 +293,15 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception {
     // 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,8 @@ public void printWarningIfCacheIsDown() throws Exception {
     StoredEventHandler eventHandler = new StoredEventHandler();
     reporter.addHandler(eventHandler);
 
-    RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor(reporter);
+    RemoteSpawnRunner runner = newSpawnRunner(reporter);
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
 
     Spawn spawn = newSimpleSpawn();
     SpawnExecutionContext policy =
@@ -428,12 +404,13 @@ public void printWarningIfCacheIsDown() throws Exception {
 
   @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();
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
 
     Spawn spawn = newSimpleSpawn();
     SpawnExecutionContext policy =
@@ -452,12 +429,13 @@ public void noRemoteExecutorFallbackFails() throws Exception {
 
   @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();
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
 
     Spawn spawn = newSimpleSpawn();
     SpawnExecutionContext policy =
@@ -1007,6 +985,14 @@ private RemoteSpawnRunner newSpawnRunner() {
         /* 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 @@ private RemoteSpawnRunner newSpawnRunner(
         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 f45b14c4e154ed..cb6981b50ded26 100755
--- a/src/test/shell/integration/execution_strategies_test.sh
+++ b/src/test/shell/integration/execution_strategies_test.sh
@@ -44,11 +44,6 @@ fi
 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() {