Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.0.0] Use backslashes in executable paths when remotely executing on Windows #24080

Merged
merged 1 commit into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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
Loading