Use backslashes in executable paths when remotely executing on Windows

`.bat` scripts can only be executed on Windows when their paths use backslashes, not forward slashes. For this reason, local execution carefully replaces forward slashes with backslashes in the executable of a `Spawn` when executing on Windows.

This commit adds equivalent logic for the case of remote execution on Windows from any host:
* Remote execution replaces forward slashes with backslashes in the first argument when executing on Windows.
* Most calls to `PathFragment#getCallablePathString` are replaced with the new execution platform aware `getCallablePathStringForOs`. This affects test actions, in which various wrappers execute different executables (e.g. `--run_under` targets) and thus can have Bazel-controlled executable path strings in locations other than the `argv[0]`.

Fixes #11636

Closes #23986.

PiperOrigin-RevId: 689357323
Change-Id: Ifb842babc02c6d741d3b45914a5bf5c032204e2b

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java

@@ -13,7 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.constraints;
 
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableBiMap;
+import com.google.devtools.build.lib.analysis.platform.ConstraintCollection;
 import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -29,8 +30,12 @@ public final class ConstraintConstants {
           Label.parseCanonicalUnchecked("@platforms//os:os"));
 
   // Standard mapping between OS and the corresponding platform constraints.
-  public static final ImmutableMap<OS, ConstraintValueInfo> OS_TO_CONSTRAINTS =
-      ImmutableMap.<OS, ConstraintValueInfo>builder()
+  public static final ImmutableBiMap<OS, ConstraintValueInfo> OS_TO_CONSTRAINTS =
+      ImmutableBiMap.<OS, ConstraintValueInfo>builder()
+          .put(
+              OS.LINUX,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:linux")))
           .put(
               OS.DARWIN,
               ConstraintValueInfo.create(
@@ -58,6 +63,19 @@ public final class ConstraintConstants {
                   Label.parseCanonicalUnchecked("@platforms//os:none")))
           .buildOrThrow();
 
+  /**
+   * Returns the OS corresponding to the given constraint collection based on the contained platform
+   * constraint.
+   */
+  public static OS getOsFromConstraints(ConstraintCollection constraintCollection) {
+    if (!constraintCollection.has(OS_CONSTRAINT_SETTING)) {
+      return OS.getCurrent();
+    }
+    return OS_TO_CONSTRAINTS
+        .inverse()
+        .getOrDefault(constraintCollection.get(OS_CONSTRAINT_SETTING), OS.getCurrent());
+  }
+
   // No-op constructor to keep this from being instantiated.
   private ConstraintConstants() {}
 }

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -433,8 +433,7 @@ private TestParams createTestAction()
                 run,
                 config,
                 ruleContext.getWorkspaceName(),
-                (!isUsingTestWrapperInsteadOfTestSetupScript
-                        || executionSettings.needsShell(ruleContext.isExecutedOnWindows()))
+                (!isUsingTestWrapperInsteadOfTestSetupScript || executionSettings.needsShell())
                     ? ShToolchain.getPathForPlatform(
                         ruleContext.getConfiguration(), ruleContext.getExecutionPlatform())
                     : null,
@@ -447,8 +446,7 @@ private TestParams createTestAction()
                 MoreObjects.firstNonNull(
                     Allowlist.fetchPackageSpecificationProviderOrNull(
                         ruleContext, "external_network"),
-                    PackageSpecificationProvider.EMPTY),
-                ruleContext.isExecutedOnWindows());
+                    PackageSpecificationProvider.EMPTY));
 
         testOutputs.addAll(testRunnerAction.getSpawnOutputs());
         testOutputs.addAll(testRunnerAction.getOutputs());

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -147,8 +147,6 @@ public class TestRunnerAction extends AbstractAction
   private final int runNumber;
   private final String workspaceName;
 
-  private final boolean isExecutedOnWindows;
-
   /**
    * Cached test result status used to minimize disk accesses. This field is set when test status is
    * retrieved from disk or saved to disk. This field is null if it has not been set yet. This field
@@ -216,8 +214,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       boolean splitCoveragePostProcessing,
       NestedSetBuilder<Artifact> lcovMergerFilesToRun,
       @Nullable Artifact lcovMergerRunfilesMiddleman,
-      PackageSpecificationProvider networkAllowlist,
-      boolean isExecutedOnWindows) {
+      PackageSpecificationProvider networkAllowlist) {
     super(
         owner,
         inputs,
@@ -308,8 +305,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
             getUndeclaredOutputsDir(),
             undeclaredOutputsAnnotationsDir,
             baseDir.getRelative("test_attempts"));
-
-    this.isExecutedOnWindows = isExecutedOnWindows;
   }
 
   public boolean allowLocalTests() {
@@ -326,10 +321,6 @@ public boolean mayModifySpawnOutputsAfterExecution() {
     return true;
   }
 
-  public boolean isExecutedOnWindows() {
-    return isExecutedOnWindows;
-  }
-
   public Artifact getRunfilesMiddleman() {
     return runfilesMiddleman;
   }
@@ -728,7 +719,10 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
     env.put("TEST_WORKSPACE", getRunfilesPrefix());
     env.put(
         "TEST_BINARY",
-        getExecutionSettings().getExecutable().getRunfilesPath().getCallablePathString());
+        getExecutionSettings()
+            .getExecutable()
+            .getRunfilesPath()
+            .getCallablePathStringForOs(executionSettings.getExecutionOs()));
 
     // When we run test multiple times, set different TEST_RANDOM_SEED values for each run.
     // Don't override any previous setting.

src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java

@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.TestAction.Code;
 import com.google.devtools.build.lib.shell.TerminationStatus;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -211,12 +212,17 @@ public static ImmutableList<String> getArgs(TestRunnerAction testAction)
   public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction testAction)
       throws CommandLineExpansionException, InterruptedException {
     List<String> args = Lists.newArrayList();
+    OS executionOs = testAction.getExecutionSettings().getExecutionOs();
 
     Artifact testSetup = testAction.getTestSetupScript();
-    args.add(testSetup.getExecPath().getCallablePathString());
+    args.add(testSetup.getExecPath().getCallablePathStringForOs(executionOs));
 
     if (testAction.isCoverageMode()) {
-      args.add(testAction.getCollectCoverageScript().getExecPathString());
+      args.add(
+          testAction
+              .getCollectCoverageScript()
+              .getExecPath()
+              .getCallablePathStringForOs(executionOs));
     }
 
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
@@ -227,21 +233,28 @@ public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction test
     }
 
     // Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia.
+    // Do not use getCallablePathStringForOs as tw.exe expects a path with forward slashes.
     args.add(execSettings.getExecutable().getRunfilesPath().getCallablePathString());
     Iterables.addAll(args, execSettings.getArgs().arguments());
     return ImmutableList.copyOf(args);
   }
 
   private static void addRunUnderArgs(TestRunnerAction testAction, List<String> args) {
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
+    OS executionOs = execSettings.getExecutionOs();
     if (execSettings.getRunUnderExecutable() != null) {
-      args.add(execSettings.getRunUnderExecutable().getRunfilesPath().getCallablePathString());
+      args.add(
+          execSettings
+              .getRunUnderExecutable()
+              .getRunfilesPath()
+              .getCallablePathStringForOs(executionOs));
     } else {
-      if (execSettings.needsShell(testAction.isExecutedOnWindows())) {
+      if (execSettings.needsShell()) {
         // TestActionBuilder constructs TestRunnerAction with a 'null' shell only when none is
         // required. Something clearly went wrong.
         Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction);
-        String shellExecutable = testAction.getShExecutableMaybe().getPathString();
+        String shellExecutable =
+            testAction.getShExecutableMaybe().getCallablePathStringForOs(executionOs);
         args.add(shellExecutable);
         args.add("-c");
         args.add("\"$@\"");

src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java

@@ -25,7 +25,9 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
+import com.google.devtools.build.lib.analysis.constraints.ConstraintConstants;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 import javax.annotation.Nullable;
 
@@ -48,6 +50,7 @@ public final class TestTargetExecutionSettings {
   private final Artifact runfilesInputManifest;
   private final Artifact instrumentedFileManifest;
   private final boolean testRunnerFailFast;
+  private final OS executionOs;
 
   TestTargetExecutionSettings(
       RuleContext ruleContext,
@@ -79,6 +82,8 @@ public final class TestTargetExecutionSettings {
     this.runfiles = runfilesSupport.getRunfiles();
     this.runfilesInputManifest = runfilesSupport.getRunfilesInputManifest();
     this.instrumentedFileManifest = instrumentedFileManifest;
+    this.executionOs =
+        ConstraintConstants.getOsFromConstraints(ruleContext.getExecutionPlatform().constraints());
   }
 
   @Nullable
@@ -156,7 +161,11 @@ public Artifact getInstrumentedFileManifest() {
     return instrumentedFileManifest;
   }
 
-  public boolean needsShell(boolean executedOnWindows) {
+  public OS getExecutionOs() {
+    return executionOs;
+  }
+
+  public boolean needsShell() {
     RunUnder r = getRunUnder();
     if (r == null) {
       return false;
@@ -168,6 +177,6 @@ public boolean needsShell(boolean executedOnWindows) {
     // --run_under commands that do not contain '/' are either shell built-ins or need to be
     // located on the PATH env, so we wrap them in a shell invocation. Note that we shell-tokenize
     // the --run_under parameter and getCommand only returns the first such token.
-    return !command.contains("/") && (!executedOnWindows || !command.contains("\\"));
+    return !command.contains("/") && (!executionOs.equals(OS.WINDOWS) || !command.contains("\\"));
   }
 }

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

@@ -443,7 +443,10 @@ private static Spawn createXmlGeneratingSpawn(
       TestRunnerAction action, ImmutableMap<String, String> testEnv, SpawnResult result) {
     ImmutableList<String> args =
         ImmutableList.of(
-            action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(),
+            action
+                .getTestXmlGeneratorScript()
+                .getExecPath()
+                .getCallablePathStringForOs(action.getExecutionSettings().getExecutionOs()),
             action.getTestLog().getExecPathString(),
             action.getXmlOutputPath().getPathString(),
             Integer.toString(result.getWallTimeInMs() / 1000),

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -72,6 +72,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:constraints/constraint_constants",
+        "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
         "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper",
@@ -114,11 +116,13 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:exit_code",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/util:string",
         "//src/main/java/com/google/devtools/build/lib/util:temp_path_generator",
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/util/io:out-err",
         "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:ospathpolicy",
         "//src/main/java/com/google/devtools/build/lib/vfs:output_service",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -74,6 +74,8 @@
 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.analysis.constraints.ConstraintConstants;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformUtils;
 import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
 import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent;
@@ -109,10 +111,12 @@
 import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput;
 import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.TempPathGenerator;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.OsPathPolicy;
 import com.google.devtools.build.lib.vfs.OutputService;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -252,7 +256,8 @@ private Command buildCommand(
       ImmutableMap<String, String> env,
       @Nullable Platform platform,
       RemotePathResolver remotePathResolver,
-      @Nullable SpawnScrubber spawnScrubber) {
+      @Nullable SpawnScrubber spawnScrubber,
+      @Nullable PlatformInfo executionPlatform) {
     Command.Builder command = Command.newBuilder();
     if (useOutputPaths) {
       var outputPaths = new ArrayList<String>();
@@ -283,10 +288,16 @@ private Command buildCommand(
     if (platform != null) {
       command.setPlatform(platform);
     }
+    boolean first = true;
     for (String arg : arguments) {
       if (spawnScrubber != null) {
         arg = spawnScrubber.transformArgument(arg);
       }
+      if (first && executionPlatform != null) {
+        first = false;
+        OS executionOs = ConstraintConstants.getOsFromConstraints(executionPlatform.constraints());
+        arg = OsPathPolicy.of(executionOs).postProcessPathStringForExecution(arg);
+      }
       command.addArguments(reencodeInternalToExternal(arg));
     }
     // Sorting the environment pairs by variable name.
@@ -646,7 +657,8 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
               spawn.getEnvironment(),
               platform,
               remotePathResolver,
-              spawnScrubber);
+              spawnScrubber,
+              spawn.getExecutionPlatform());
       Digest commandHash = digestUtil.compute(command);
       Action action =
           Utils.buildAction(

src/main/java/com/google/devtools/build/lib/vfs/BUILD

@@ -48,6 +48,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//third_party:error_prone_annotations",
         "//third_party:guava",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java

@@ -86,9 +86,17 @@ public interface OsPathPolicy {
 
   boolean isCaseSensitive();
 
+  /**
+   * Modifies the given string to be suitable for execution on the OS represented by this policy.
+   */
+  String postProcessPathStringForExecution(String callablePathString);
+
+  static OsPathPolicy of(OS os) {
+    return os == OS.WINDOWS ? WindowsOsPathPolicy.INSTANCE : UnixOsPathPolicy.INSTANCE;
+  }
+
   // We *should* use a case-insensitive policy for OS.DARWIN, but we currently don't handle this.
-  OsPathPolicy HOST_POLICY =
-      OS.getCurrent() == OS.WINDOWS ? WindowsOsPathPolicy.INSTANCE : UnixOsPathPolicy.INSTANCE;
+  OsPathPolicy HOST_POLICY = of(OS.getCurrent());
 
   static OsPathPolicy getFilePathOs() {
     return HOST_POLICY;

src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java

@@ -622,6 +622,8 @@ public String getSafePathString() {
    *
    * <p>In this way, a shell will always interpret such a string as path (absolute or relative to
    * the working directory) and not as command to be searched for in the search path.
+   *
+   * <p>Prefer {@link #getCallablePathStringForOs} if the execution OS is available.
    */
   public String getCallablePathString() {
     if (isAbsolute()) {
@@ -635,6 +637,18 @@ public String getCallablePathString() {
     }
   }
 
+  /**
+   * Returns the path string using the native name-separator for the given OS, but does so in a way
+   * unambiguously recognizable as path. In other words, return "." for relative and empty paths,
+   * and prefix relative paths with an additional "." segment.
+   *
+   * <p>In this way, a shell will always interpret such a string as path (absolute or relative to
+   * the working directory) and not as command to be searched for in the search path.
+   */
+  public String getCallablePathStringForOs(com.google.devtools.build.lib.util.OS executionOs) {
+    return OsPathPolicy.of(executionOs).postProcessPathStringForExecution(getCallablePathString());
+  }
+
   /**
    * Returns the file extension of this path, excluding the period, or "" if there is no extension.
    */

src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java

@@ -139,4 +139,9 @@ public char additionalSeparator() {
   public boolean isCaseSensitive() {
     return true;
   }
+
+  @Override
+  public String postProcessPathStringForExecution(String callablePathString) {
+    return callablePathString;
+  }
 }