[8.4.0] Skip strategies incompatible with path mapping during resolution (bazelbuild#26563)

Spawn strategies that aren't sandboxed are now skipped while resolving a
strategy for a path mapped spawn, which allows non-sandboxed strategies
to be listed first and also results in a better error message if no
compatible strategy is available (instead of an obscure exec failure).

Worker execution is special in that it can always sandbox if needed, but
may not do so based on options. For this purpose, path mapped spawns are
now treated just like dynamically executed spawns by forcing sandboxing.

In the future, the same mechanism can potentially be used to run spawns
for rewound actions in Bazel, which must not delete or override their
previous outputs.

Work towards
bazelbuild#22658 (comment)
Fixes bazel-contrib/rules_go#4366

Closes bazelbuild#25430.

PiperOrigin-RevId: 784040555
Change-Id: I26e110801ce84c41aef5f3e03e34879c595ebedf

Commit
bazelbuild@a42c981

Co-authored-by: Fabian Meumertzheim <[email protected]>

Author: bazel-io

src/main/java/com/google/devtools/build/lib/actions/Spawns.java

@@ -67,6 +67,14 @@ public static boolean mayBeSandboxed(Spawn spawn) {
         && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LOCAL);
   }
 
+  /**
+   * Returns whether a Spawn must be executed on a separate exec root (i.e., in a sandbox) since it
+   * references rewritten input and output paths.
+   */
+  public static boolean usesPathMapping(Spawn spawn) {
+    return !spawn.getPathMapper().isNoop();
+  }
+
   /** Returns whether a Spawn needs network access in order to run successfully. */
   public static boolean requiresNetwork(Spawn spawn, boolean defaultSandboxDisallowNetwork) {
     if (spawn.getExecutionInfo().containsKey(ExecutionRequirements.BLOCK_NETWORK)) {

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java

@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnStrategy;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
@@ -86,11 +87,15 @@ public List<? extends SpawnStrategy> resolve(
       if (fallbackStrategies.isEmpty()) {
         String message =
             String.format(
-                "%s spawn cannot be executed with any of the available strategies: %s. Your"
+                "%s spawn%s cannot be executed with any of the available strategies: %s. Your"
                     + " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably"
                     + " too strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for"
                     + " advice",
-                spawn.getMnemonic(), strategies);
+                spawn.getMnemonic(),
+                Spawns.usesPathMapping(spawn)
+                    ? ", which requires sandboxing due to path mapping,"
+                    : "",
+                strategies);
         throw new UserExecException(
             FailureDetail.newBuilder()
                 .setMessage(message)

src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java

@@ -170,7 +170,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
 
   @Override
   public boolean canExec(Spawn spawn) {
-    return true;
+    return !Spawns.usesPathMapping(spawn);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
@@ -489,6 +490,9 @@ public boolean canExec(Spawn spawn) {
 
     @Override
     public boolean canExecWithLegacyFallback(Spawn spawn) {
+      if (Spawns.usesPathMapping(spawn)) {
+        return false;
+      }
       boolean canExec = !sandboxSpawnRunner.canExec(spawn) && fallbackSpawnRunner.canExec(spawn);
       if (canExec) {
         // We give a warning to use strategies instead, whether or not we allow the fallback

src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java

@@ -132,16 +132,21 @@ static WorkerKey createWorkerKey(
       boolean dynamic,
       WorkerProtocolFormat protocolFormat) {
     String workerKeyMnemonic = Spawns.getWorkerKeyMnemonic(spawn);
-    boolean multiplex = options.workerMultiplex && Spawns.supportsMultiplexWorkers(spawn);
-    if (dynamic && !(Spawns.supportsMultiplexSandboxing(spawn) && options.multiplexSandboxing)) {
-      multiplex = false;
-    }
+    boolean mustSandbox = dynamic || Spawns.usesPathMapping(spawn);
+    boolean shouldMultiplex = options.workerMultiplex && Spawns.supportsMultiplexWorkers(spawn);
+    boolean canSandboxMultiplex =
+        options.multiplexSandboxing && Spawns.supportsMultiplexSandboxing(spawn);
     boolean sandboxed;
-    if (multiplex) {
-      sandboxed =
-          Spawns.supportsMultiplexSandboxing(spawn) && (options.multiplexSandboxing || dynamic);
+    boolean multiplex;
+    if (mustSandbox) {
+      sandboxed = true;
+      multiplex = shouldMultiplex && canSandboxMultiplex;
+    } else if (shouldMultiplex) {
+      sandboxed = canSandboxMultiplex;
+      multiplex = true;
     } else {
-      sandboxed = options.workerSandboxing || dynamic;
+      sandboxed = options.workerSandboxing;
+      multiplex = false;
     }
     boolean useInMemoryTracking = false;
     if (sandboxed) {

src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java

@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
+import com.google.devtools.build.lib.actions.PathMapper;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
@@ -106,6 +107,7 @@ public void setUp() throws Exception {
         .registerWorker(
             anyInt(), anyLong(), any(), anyString(), anyBoolean(), anyBoolean(), anyInt(), any());
     when(spawn.getLocalResources()).thenReturn(ResourceSet.createWithRamCpu(100, 1));
+    when(spawn.getPathMapper()).thenReturn(PathMapper.NOOP);
     when(resourceManager.acquireResources(any(), any(), any())).thenReturn(resourceHandle);
     when(resourceHandle.getWorker()).thenReturn(worker);
   }

src/test/shell/bazel/path_mapping_test.sh

@@ -93,6 +93,17 @@ function tear_down() {
   stop_worker
 }
 
+function test_path_stripping_local_fails() {
+  cache_dir=$(mktemp -d)
+
+  bazel build -c fastbuild \
+    --disk_cache=$cache_dir \
+    --experimental_output_paths=strip \
+    --strategy=Javac=local \
+    //src/main/java/com/example:Main &> $TEST_log && fail "build succeeded unexpectedly"
+  expect_log 'Javac spawn, which requires sandboxing due to path mapping, cannot be executed with any of the available strategies'
+}
+
 function test_path_stripping_sandboxed() {
   if is_windows; then
     echo "Skipping test_path_stripping_sandboxed on Windows as it requires sandboxing"
@@ -101,10 +112,12 @@ function test_path_stripping_sandboxed() {
 
   cache_dir=$(mktemp -d)
 
+  # Validate that the sandboxed strategy is preferred over the local strategy
+  # with path mapping.
   bazel run -c fastbuild \
     --disk_cache=$cache_dir \
     --experimental_output_paths=strip \
-    --strategy=Javac=sandboxed \
+    --strategy=Javac=local,sandboxed \
     //src/main/java/com/example:Main &> $TEST_log || fail "run failed unexpectedly"
   expect_log 'Hello, World!'
   # JavaToolchainCompileBootClasspath, JavaToolchainCompileClasses, 1x header compilation and 2x
@@ -123,19 +136,14 @@ function test_path_stripping_sandboxed() {
 }
 
 function test_path_stripping_singleplex_worker() {
-  if is_windows; then
-    echo "Skipping test_path_stripping_singleplex_worker on Windows as it requires sandboxing"
-    return
-  fi
-
   cache_dir=$(mktemp -d)
 
+  # Worker sandboxing is enabled automatically, multiplexing is disabled since
+  # the default toolchain does not support sandboxing yet.
   bazel run -c fastbuild \
     --disk_cache=$cache_dir \
     --experimental_output_paths=strip \
     --strategy=Javac=worker \
-    --worker_sandboxing \
-    --noexperimental_worker_multiplex \
     //src/main/java/com/example:Main &> $TEST_log || fail "run failed unexpectedly"
   expect_log 'Hello, World!'
   # JavaToolchainCompileBootClasspath, JavaToolchainCompileClasses and header compilation.
@@ -148,8 +156,6 @@ function test_path_stripping_singleplex_worker() {
     --disk_cache=$cache_dir \
     --experimental_output_paths=strip \
     --strategy=Javac=worker \
-    --worker_sandboxing \
-    --noexperimental_worker_multiplex \
     //src/main/java/com/example:Main &> $TEST_log || fail "run failed unexpectedly"
   expect_log 'Hello, World!'
   expect_log '5 disk cache hit'