Skip to content

Commit

Permalink
Use backslashes in executable paths when remotely executing on Windows
Browse files Browse the repository at this point in the history
`.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
  • Loading branch information
fmeum authored and copybara-github committed Oct 24, 2024
1 parent 02b4dd7 commit 9c90100
Show file tree
Hide file tree
Showing 15 changed files with 150 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand Down Expand Up @@ -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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -308,8 +305,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
getUndeclaredOutputsDir(),
undeclaredOutputsAnnotationsDir,
baseDir.getRelative("test_attempts"));

this.isExecutedOnWindows = isExecutedOnWindows;
}

public boolean allowLocalTests() {
Expand All @@ -326,10 +321,6 @@ public boolean mayModifySpawnOutputsAfterExecution() {
return true;
}

public boolean isExecutedOnWindows() {
return isExecutedOnWindows;
}

public Artifact getRunfilesMiddleman() {
return runfilesMiddleman;
}
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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("\"$@\"");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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("\\"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/vfs/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,4 +139,9 @@ public char additionalSeparator() {
public boolean isCaseSensitive() {
return true;
}

@Override
public String postProcessPathStringForExecution(String callablePathString) {
return callablePathString;
}
}
Loading

0 comments on commit 9c90100

Please sign in to comment.