Repository: bazelbuild/bazel
Issue: incompatible_list_based_execution_strategy_selection: List-based explicit spawn strategy selection.
Labels: incompatible-change
team-Remote-Exec

For more detailed description visit our blog post: https://blog.bazel.build/2019/06/19/list-strategy.html

## :no_good_man: Current situation:
- The user can set the execution strategy in general and for individual action mnemonics using the flags `--spawn_strategy=`, `--strategy=Mnemonic=` and `--strategy_regexp=Regex=`. The flags take a single strategy name as value. Example: `--spawn_strategy=linux-sandbox`.
- Bazel has a hardcoded and undocumented list of mnemonics that are known to work well with certain strategies and uses these unless the user overrides them *individually* using `--strategy=Mnemonic=` flags. Example: Bazel will use some kind of sandboxing by default if its available on your system, otherwise non-sandboxed execution (e.g. on Windows). However, it will run Javac actions via the persistent worker strategy.
- Each strategy has custom code to deal with the situation that it can't execute a given action - for example, the remote strategy silently falls back to sandboxed execution if an action can't run remotely. The sandbox strategy silently falls back to non-sandboxed local execution if an action doesn't support sandboxing. There's no good way to configure this behavior.

## :new: The option `--incompatible_list_based_execution_strategy_selection`:
- Allows the user to pass comma-separated lists of strategies to the above mentioned flags: `--spawn_strategy=remote,worker,linux-sandbox`.
- Each strategy now knows whether it can execute a given action. For any action that it wants to execute, Bazel just picks the first strategy from the given list that claims to be able to execute the action. If no strategy remains, the execution will fail.
 - This means you can now have your build fail if any non-sandboxable or non-remotable action sneaks in! More reproducibility and safety for your builds! :tada:
- The strategies no longer do their own custom fallback, simplifying the code and unifying the behavior.

## :bulb: Some ideas how to use this:
- Don't set any flags and Bazel will try to do the best automatically: Use remote execution if it's available, otherwise persistent workers, otherwise sandboxed execution, otherwise non-sandboxed execution.
- I want the best sandboxed build on my Linux machine and no automatic fallback to symlink-only sandboxing, non-sandboxed execution and no persistent workers: `--spawn_strategy=linux-sandbox`.
- I want persistent workers for actions that support it, but otherwise only sandboxed execution: `--spawn_strategy=worker,sandboxed`.

## :construction: Migration needed:
- If you currently use the strategy selection flags, you'll want to revisit the values you set them to.
- If you currently patch your Bazel version to remove fallback to non-sandboxed execution, this should no longer be necessary then (:wave: @AustinSchuh).
- If you currently use remote execution with `--strategy=remote` and/or `--spawn_strategy=remote` and have actions that might not be executed remotely, consider removing those `strategy` flags completely, in this case bazel will pickup the first available strategy from the default list, which is `remote,worker,sandboxed,local`. Alternatively, add a strategy to fallback to if remote is not possible for an action, for example `--spawn_strategy=remote,sandboxed` will fallback to a sandboxed execution if remote is not possible.
- If you are using `--config=remote` you might need to change your .bazelrc file, where all the `remote` configs are located, including `strategy` flags.

Issue comments:
/sub
Thanks @philwo !  This looks helpful.

We moved over to just using ```--spawn_strategy=linux-sandbox``` based on your advice in late 2017.  That means we no longer have a patch for that.

Does this replace ```--remote_local_fallback_strategy=linux-sandbox``` ?
Yes.
Under `incompatible_list_based_execution_strategy_selection`, `remote_local_fallback_strategy` will no longer be respected. A target that is marked no-remote will not be eligible for remote execution, so the next available strategy from the list will be applied.

We plan to deprecate `remote_local_fallback_strategy` by the time `incompatible_list_based_execution_strategy_selection` becomes the default.

We think that the new strategy list should serve all the fallback needs (and make things more explicit and cleaner). Do let us know if for some reason your case is not being served!
@ishikhman as parts of your flag work can you please take over ownership of flipping this flag? You probably won't get to it by 0.26 (this thursday) so let's target to flip this in 0.27? I don't expect there to be much trouble except for fixing a few tests.
Could you improve the release notes about it?
See https://github.com/bazelbuild/bazel/issues/7816#issuecomment-502304725
> Could you improve the release notes about it?
> See [#7816 (comment)](https://github.com/bazelbuild/bazel/issues/7816#issuecomment-502304725)

hm, it is in release notes (I've checked https://github.com/bazelbuild/bazel-blog/pull/178) and blog post is coming right after 0.27 release (https://github.com/bazelbuild/bazel-blog/pull/173). 

The release notes are already rather long, so what I can suggest is to include a link to the blog post. WDYT?
Sounds good to me.
> Yes.
> Under `incompatible_list_based_execution_strategy_selection`, `remote_local_fallback_strategy` will no longer be respected. A target that is marked no-remote will not be eligible for remote execution, so the next available strategy from the list will be applied.
> 
> We plan to deprecate `remote_local_fallback_strategy` by the time `incompatible_list_based_execution_strategy_selection` becomes the default.
> 
> We think that the new strategy list should serve all the fallback needs (and make things more explicit and cleaner). Do let us know if for some reason your case is not being served!

@agoulti @buchgr I've just checked that this is no _exactly_ the case. There are 2 use cases where `remote_local_fallback_strategy` was used:
1) when target is marked as `no-remote` or `no-remote-exec`: [link](https://github.com/bazelbuild/bazel/blob/11a98a51095bfda494ddf7ba69d8ad80309ef63c/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java#L181)
2) when remote execution failed and was caused NOT by a Timeout exception: [link](https://github.com/bazelbuild/bazel/blob/11a98a51095bfda494ddf7ba69d8ad80309ef63c/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java#L281) and [link2](https://github.com/bazelbuild/bazel/blob/11a98a51095bfda494ddf7ba69d8ad80309ef63c/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java#L216)

The first case is fully covered by a list-based strategy, but not the second one. Therefore if the second use case is important (which I think it is, because someone mentioned that they rely on fallback in case of connection problems to a remote system) we cannot discard `remote_local_fallback_strategy`.

I'll remove the usages for the first case as a part of #8970.

If we want to completely deprecate `remote_local_fallback_strategy`, i think, we should consider generic fallback mechanism that would be using a provided list.


Pull request review comments:

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 843178d3669721..205e5eb54284f7 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,7 +59,7 @@ public class ExecutionOptions extends OptionsBase {
 
   @Option(
       name = "incompatible_list_based_execution_strategy_selection",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
       effectTags = {OptionEffectTag.EXECUTION},
       metadataTags = {


Test patch:
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 4fe983c09be561..7e1c8a0b460838 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -104,7 +104,6 @@ EOF
 
   bazel clean >& $TEST_log
   bazel build \
-      --incompatible_list_based_execution_strategy_selection \
       --remote_executor=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote execution"
@@ -261,10 +260,7 @@ EOF
       --remote_local_fallback_strategy=local \
       --build_event_text_file=gen1.log \
       //gen1 >& $TEST_log \
-      || fail "Expected success"
-
-  mv gen1.log $TEST_log
-  expect_log "1 process: 1 local"
+      && fail "Expected failure" || true
 }
 
 function test_local_fallback_with_local_strategy_lists() {
@@ -280,7 +276,6 @@ tags = ["no-remote"],
 EOF
 
   bazel build \
-      --incompatible_list_based_execution_strategy_selection \
       --spawn_strategy=remote,local \
       --remote_executor=localhost:${worker_port} \
       --build_event_text_file=gen1.log \
@@ -304,7 +299,6 @@ tags = ["no-remote"],
 EOF
 
   bazel build \
-      --incompatible_list_based_execution_strategy_selection \
       --spawn_strategy=remote,sandboxed,local \
       --remote_executor=localhost:${worker_port} \
       --build_event_text_file=gen1.log \
@@ -328,7 +322,6 @@ tags = ["no-remote"],
 EOF
 
   bazel build \
-      --incompatible_list_based_execution_strategy_selection \
       --remote_executor=localhost:${worker_port} \
       --build_event_text_file=gen1.log \
       //gen1 >& $TEST_log \
@@ -356,10 +349,7 @@ EOF
       --remote_local_fallback_strategy=sandboxed \
       --build_event_text_file=gen2.log \
       //gen2 >& $TEST_log \
-      || fail "Expected success"
-
-  mv gen2.log $TEST_log
-  expect_log "1 process: 1 .*-sandbox"
+      && fail "Expected failure" || true
 }
 
 function is_file_uploaded() {
@@ -903,7 +893,7 @@ EOF
   || fail "Expected bazel-bin/a/remote.txt to have not been downloaded"
 
   bazel build \
-    --genrule_strategy=remote \
+    --genrule_strategy=remote,local \
     --remote_executor=localhost:${worker_port} \
     --experimental_inmemory_jdeps_files \
     --experimental_inmemory_dotd_files \
diff --git a/src/test/shell/integration/bazel_worker_test.sh b/src/test/shell/integration/bazel_worker_test.sh
index c7202cf0acf24c..1e17705f15648e 100755
--- a/src/test/shell/integration/bazel_worker_test.sh
+++ b/src/test/shell/integration/bazel_worker_test.sh
@@ -460,16 +460,15 @@ EOF
   sed -i.bak '/execution_requirements/d' work.bzl
   rm -f work.bzl.bak
 
-  bazel build --worker_quit_after_build :hello_world &> $TEST_log \
-    || fail "build failed"
+  bazel build --worker_quit_after_build :hello_world &> $TEST_log && fail "Failure expected" || true
 
-  expect_log "Worker strategy cannot execute this Work action, because the action's execution info does not contain 'supports-workers=1'"
+  expect_log "No usable spawn strategy found for spawn with mnemonic Work"
   expect_not_log "Created new ${WORKER_TYPE_LOG_STRING} Work worker (id [0-9]\+)"
   expect_not_log "Destroying Work worker (id [0-9]\+)"
 
   # WorkerSpawnStrategy falls back to standalone strategy, so we still expect the output to be generated.
   [ -e "$BINS/hello_world.out" ] \
-    || fail "Worker did not produce output"
+    && fail "Worker was not supposed to produce output" || true
 }
 
 function test_environment_is_clean() {
diff --git a/src/test/shell/integration/execution_strategies_test.sh b/src/test/shell/integration/execution_strategies_test.sh
index c312be660f9c3a..5e81b6d63eb017 100755
--- a/src/test/shell/integration/execution_strategies_test.sh
+++ b/src/test/shell/integration/execution_strategies_test.sh
@@ -44,16 +44,22 @@ fi
 source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
   || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
 
-# Tests that you have to opt-in to list based strategy selection via an incompatible flag.
+# Tests that you cat opt-out from a list based strategy selection via an incompatible flag.
 function test_incompatible_flag_required() {
-  bazel build --spawn_strategy=worker,local --debug_print_action_contexts &> $TEST_log || true
+  bazel build --spawn_strategy=worker,local --debug_print_action_contexts \
+  --incompatible_list_based_execution_strategy_selection=false &> $TEST_log || true
   expect_log "incompatible_list_based_execution_strategy_selection was not enabled"
 }
 
+# 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() {
-  bazel build --incompatible_list_based_execution_strategy_selection \
-      --spawn_strategy=worker,local --debug_print_action_contexts &> $TEST_log || fail
+  bazel build --spawn_strategy=worker,local --debug_print_action_contexts &> $TEST_log || fail
   # Can't test for exact strategy names here, because they differ between platforms and products.
   expect_log "\"\" = \[.*, .*\]"
 }
@@ -61,8 +67,7 @@ function test_multiple_strategies() {
 # Tests that the hardcoded Worker strategies are not introduced with the new
 # strategy selection
 function test_no_worker_defaults() {
-  bazel build --incompatible_list_based_execution_strategy_selection \
-      --debug_print_action_contexts &> $TEST_log || fail
+  bazel build --debug_print_action_contexts &> $TEST_log || fail
   # Can't test for exact strategy names here, because they differ between platforms and products.
   expect_not_log "\"Closure\""
   expect_not_log "\"DexBuilder\""
@@ -71,21 +76,18 @@ function test_no_worker_defaults() {
 
 # Tests that Bazel catches an invalid strategy list that has an empty string as an element.
 function test_empty_strategy_in_list_is_forbidden() {
-  bazel build --incompatible_list_based_execution_strategy_selection \
-      --spawn_strategy=worker,,local --debug_print_action_contexts &> $TEST_log || true
+  bazel build --spawn_strategy=worker,,local --debug_print_action_contexts &> $TEST_log || true
   expect_log "--spawn_strategy=worker,,local: Empty values are not allowed as part of this comma-separated list of options"
 }
 
 # Test that when you set a strategy to the empty string, it gets removed from the map of strategies
 # and thus results in the default strategy being used (the one set via --spawn_strategy=).
 function test_empty_strategy_means_default() {
-  bazel build --incompatible_list_based_execution_strategy_selection \
-      --spawn_strategy=worker,local --strategy=FooBar=local \
+  bazel build --spawn_strategy=worker,local --strategy=FooBar=local \
       --debug_print_action_contexts &> $TEST_log || fail
   expect_log "\"FooBar\" = "
 
-  bazel build --incompatible_list_based_execution_strategy_selection \
-      --spawn_strategy=worker,local --strategy=FooBar=local --strategy=FooBar= \
+  bazel build --spawn_strategy=worker,local --strategy=FooBar=local --strategy=FooBar= \
       --debug_print_action_contexts &> $TEST_log || fail
   expect_not_log "\"FooBar\" = "
 }