From 356a32164243166dd412ed95ed872f5ec79a862e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 24 Oct 2024 06:30:13 -0700 Subject: [PATCH] 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 --- .../constraints/ConstraintConstants.java | 24 ++++++++++++++--- .../lib/analysis/test/TestActionBuilder.java | 6 ++--- .../lib/analysis/test/TestRunnerAction.java | 16 ++++-------- .../build/lib/analysis/test/TestStrategy.java | 23 ++++++++++++---- .../test/TestTargetExecutionSettings.java | 13 ++++++++-- .../lib/exec/StandaloneTestStrategy.java | 5 +++- .../google/devtools/build/lib/remote/BUILD | 4 +++ .../lib/remote/RemoteExecutionService.java | 16 ++++++++++-- .../com/google/devtools/build/lib/vfs/BUILD | 1 + .../devtools/build/lib/vfs/OsPathPolicy.java | 12 +++++++-- .../devtools/build/lib/vfs/PathFragment.java | 14 ++++++++++ .../build/lib/vfs/UnixOsPathPolicy.java | 5 ++++ .../build/lib/vfs/WindowsOsPathPolicy.java | 13 ++++++++++ .../google/devtools/build/lib/remote/BUILD | 2 ++ .../remote/RemoteExecutionServiceTest.java | 26 +++++++++++++++++++ 15 files changed, 150 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java index 362c31fbecfd53..0cd7dcc35a3aad 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java +++ b/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_TO_CONSTRAINTS = - ImmutableMap.builder() + public static final ImmutableBiMap OS_TO_CONSTRAINTS = + ImmutableBiMap.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() {} } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index 7040015deb0052..cf9fcd5ae3bf37 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 99e4d10bbe0ccd..1575a8ee0c7992 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/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 nonNullAsSet(Artifact... artifacts) { boolean splitCoveragePostProcessing, NestedSetBuilder lcovMergerFilesToRun, @Nullable Artifact lcovMergerRunfilesMiddleman, - PackageSpecificationProvider networkAllowlist, - boolean isExecutedOnWindows) { + PackageSpecificationProvider networkAllowlist) { super( owner, inputs, @@ -308,8 +305,6 @@ private static ImmutableSet 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 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. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java index aebd288ff3b14b..e7fb7099f3a25a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java +++ b/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 getArgs(TestRunnerAction testAction) public static ImmutableList expandedArgsFromAction(TestRunnerAction testAction) throws CommandLineExpansionException, InterruptedException { List 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,6 +233,7 @@ public static ImmutableList 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); @@ -234,14 +241,20 @@ public static ImmutableList expandedArgsFromAction(TestRunnerAction test private static void addRunUnderArgs(TestRunnerAction testAction, List 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("\"$@\""); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java index e5f69dd7eae496..1ba2ebe2ecb59d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java +++ b/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("\\")); } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 1d3dff479737fa..455f422476a8f9 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -443,7 +443,10 @@ private static Spawn createXmlGeneratingSpawn( TestRunnerAction action, ImmutableMap testEnv, SpawnResult result) { ImmutableList 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), diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 5447ffde21e0f2..cef5c7b29e203f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 15b442fd3d0138..ff6012bbbfac6c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/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 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(); @@ -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( diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD index 2fbc41620cf68a..0c4503f2203e93 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java b/src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java index 5e843acc4a262e..cf575d915a4220 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java +++ b/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; diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java index f5191af2601b2a..90e7acd6f76374 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java @@ -622,6 +622,8 @@ public String getSafePathString() { * *

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. + * + *

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. + * + *

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. */ diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java index 45f1c55e0829a2..8c7ce1eb1fe943 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java +++ b/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; + } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java b/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java index 86908cfcb1664c..8e25116fa50801 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java @@ -16,6 +16,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.windows.WindowsFileOperations; import com.google.devtools.build.lib.windows.WindowsShortPath; import java.io.IOException; @@ -39,6 +40,10 @@ interface ShortPathResolver { static class DefaultShortPathResolver implements ShortPathResolver { @Override public String resolveShortPath(String path) { + if (!OS.getCurrent().equals(OS.WINDOWS)) { + // Short path resolution only makes sense on a Windows host. + return path; + } try { return WindowsFileOperations.getLongPath(path); } catch (IOException e) { @@ -240,4 +245,12 @@ public char additionalSeparator() { public boolean isCaseSensitive() { return false; } + + @Override + public String postProcessPathStringForExecution(String callablePathString) { + // On Windows, .bat scripts (and possibly others) cannot be executed with forward slashes in + // the path. Since backslashes are the standard path separator on Windows, we replace all + // forward slashes with backslashes instead of trying to enumerate these special cases. + return callablePathString.replace('/', '\\'); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 53d8963d71cf56..865e62bbaf052f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -84,8 +84,10 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", + "//src/main/java/com/google/devtools/build/lib/analysis:constraints/constraint_constants", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/analysis:symlink_entry", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 94721483f6f059..09e4d8763b2c79 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -83,6 +83,8 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.SymlinkEntry; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; +import com.google.devtools.build.lib.analysis.constraints.ConstraintConstants; +import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -111,6 +113,7 @@ import com.google.devtools.build.lib.remote.util.RxNoGlobalErrorsRule; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; +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.DigestHashFunction; @@ -2519,6 +2522,29 @@ public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkle .containsExactly("output_dir"); } + @Test + public void buildRemoteAction_executablePathConformsToPlatform(@TestParameter OS executionOs) + throws Exception { + Spawn spawn = + new SpawnBuilder("path/to/pkg/script.bat", "some/other/arg") + .withOutputs("out") + .withPlatform( + PlatformInfo.builder() + .addConstraint(ConstraintConstants.OS_TO_CONSTRAINTS.get(executionOs)) + .build()) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(remoteOptions); + + var remoteAction = service.buildRemoteAction(spawn, context); + + String expectedFirstArg = + executionOs == OS.WINDOWS ? "path\\to\\pkg\\script.bat" : "path/to/pkg/script.bat"; + assertThat(remoteAction.getCommand().getArgumentsList()) + .containsExactly(expectedFirstArg, "some/other/arg") + .inOrder(); + } + private Spawn newSpawnFromResult(RemoteActionResult result) { return newSpawnFromResult(ImmutableMap.of(), result); }