Only include sandboxed in default local strategies for dynamic exec…

…ution 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
fmeum · Jul 5, 2024 · 6e9693e · 6e9693e
1 parent b8b166e
commit 6e9693e
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 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
@@ -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
@@ -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
@@ -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
@@ -69,29 +69,33 @@ 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"));
   }
 
   @Test
   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
@@ -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."