From 6e9693e4513066e50d4558078327afe485b522c4 Mon Sep 17 00:00:00 2001
From: Googler <twerth@google.com>
Date: Mon, 24 Jun 2024 08:05:41 -0700
Subject: [PATCH] Only include `sandboxed` in default local strategies for
 dynamic execution if it actually supported.

For example, on Windows, it is currently not supported and Bazel fails to build anything when `--internal_spawn_scheduler` is set.

PiperOrigin-RevId: 646097323
Change-Id: I3b8fc7f8e48290f236749653407f0c15c918dd65
---
 .../com/google/devtools/build/lib/dynamic/BUILD |  1 -
 .../lib/dynamic/DynamicExecutionModule.java     | 17 +++++++++++------
 .../build/lib/exec/SpawnStrategyRegistry.java   |  4 ++++
 .../lib/dynamic/DynamicExecutionModuleTest.java | 12 ++++++++----
 .../integration/execution_strategies_test.sh    |  6 ++++++
 5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/BUILD b/src/main/java/com/google/devtools/build/lib/dynamic/BUILD
index 569d83c7963c3d..19dcbc96c90c83 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/BUILD
@@ -19,7 +19,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
-        "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/exec:execution_options",
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java
index 63fb097cddd62d..76b3d409390423 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java
@@ -101,17 +101,21 @@ public void beforeCommand(CommandEnvironment env) {
   }
 
   @VisibleForTesting
-  ImmutableMap<String, List<String>> getLocalStrategies(DynamicExecutionOptions options)
-      throws AbruptExitException {
+  ImmutableMap<String, List<String>> getLocalStrategies(
+      DynamicExecutionOptions options, boolean sandboxingSupported) throws AbruptExitException {
     // Options that set "allowMultiple" to true ignore the default value, so we replicate that
     // functionality here.
     ImmutableMap.Builder<String, List<String>> localAndWorkerStrategies = ImmutableMap.builder();
+    ImmutableList.Builder<String> defaultLocalStrategies = ImmutableList.builder();
+    defaultLocalStrategies.add("worker");
+    if (sandboxingSupported) {
+      defaultLocalStrategies.add("sandboxed");
+    }
     if (localOptions != null && localOptions.localLockfreeOutput) {
-      localAndWorkerStrategies.put("", ImmutableList.of("worker", "sandboxed", "standalone"));
-    } else {
       // Without local lock free, having standalone execution risks very bad performance.
-      localAndWorkerStrategies.put("", ImmutableList.of("worker", "sandboxed"));
+      defaultLocalStrategies.add("standalone");
     }
+    localAndWorkerStrategies.put("", defaultLocalStrategies.build());
 
     for (Map.Entry<String, List<String>> entry : options.dynamicLocalStrategy) {
       localAndWorkerStrategies.put(entry);
@@ -173,7 +177,8 @@ final void registerSpawnStrategies(
             jobs,
             this::canIgnoreFailure);
     registryBuilder.registerStrategy(strategy, "dynamic", "dynamic_worker");
-    registryBuilder.addDynamicLocalStrategies(getLocalStrategies(options));
+    boolean sandboxingSupported = registryBuilder.isStrategyRegistered("sandboxed");
+    registryBuilder.addDynamicLocalStrategies(getLocalStrategies(options, sandboxingSupported));
     registryBuilder.addDynamicRemoteStrategies(getRemoteStrategies(options));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
index d5c760254a2348..a715a6cebe0c6d 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
@@ -415,6 +415,10 @@ public Builder setRemoteLocalFallbackStrategyIdentifier(String commandlineIdenti
       return this;
     }
 
+    public boolean isStrategyRegistered(String strategy) {
+      return strategiesInRegistrationOrder.contains(strategy);
+    }
+
     /**
      * Finalizes the construction of the registry.
      *
diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java
index 8699096072bca7..2d969af0ad25ab 100644
--- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java
@@ -69,21 +69,25 @@ public void setUp() throws IOException, AbruptExitException {
   @Test
   public void testGetLocalStrategies_getsDefaultWithNoOptions()
       throws AbruptExitException, OptionsParsingException {
-    assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("worker,sandboxed"));
+    assertThat(module.getLocalStrategies(options, /* sandboxingSupported= */ true))
+        .isEqualTo(parseStrategies("worker,sandboxed"));
+    assertThat(module.getLocalStrategies(options, /* sandboxingSupported= */ false))
+        .isEqualTo(parseStrategies("worker"));
   }
 
   @Test
   public void testGetLocalStrategies_genericOptionOverridesFallbacks()
       throws AbruptExitException, OptionsParsingException {
     options.dynamicLocalStrategy = parseStrategiesToOptions("local,worker");
-    assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("local,worker"));
+    assertThat(module.getLocalStrategies(options, /* sandboxingSupported= */ true))
+        .isEqualTo(parseStrategies("local,worker"));
   }
 
   @Test
   public void testGetLocalStrategies_specificOptionKeepsFallbacks()
       throws AbruptExitException, OptionsParsingException {
     options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker");
-    assertThat(module.getLocalStrategies(options))
+    assertThat(module.getLocalStrategies(options, /* sandboxingSupported= */ true))
         .isEqualTo(parseStrategies("Foo=local,worker", "worker,sandboxed"));
   }
 
@@ -91,7 +95,7 @@ public void testGetLocalStrategies_specificOptionKeepsFallbacks()
   public void testGetLocalStrategies_canMixSpecificsAndGenericOptions()
       throws AbruptExitException, OptionsParsingException {
     options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker", "worker");
-    assertThat(module.getLocalStrategies(options))
+    assertThat(module.getLocalStrategies(options, /* sandboxingSupported= */ true))
         .isEqualTo(parseStrategies("Foo=local,worker", "worker"));
   }
 
diff --git a/src/test/shell/integration/execution_strategies_test.sh b/src/test/shell/integration/execution_strategies_test.sh
index 6dc1e4f6164945..14a57ba6898423 100755
--- a/src/test/shell/integration/execution_strategies_test.sh
+++ b/src/test/shell/integration/execution_strategies_test.sh
@@ -234,4 +234,10 @@ EOF
   expect_log '^remote$'
 }
 
+function test_internal_spawn_scheduler() {
+  # This is just a basic test to see whether the dynamic scheduler is setting
+  # up the correct local and remote strategies on all platforms.
+  bazel build --internal_spawn_scheduler &>"$TEST_log" || fail "build failed"
+}
+
 run_suite "Tests for the execution strategy selection."