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 bf172c6c52c964..92ef1639b45074 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 @@ -350,7 +350,6 @@ private TestParams createTestAction(int shards) { coverageArtifacts.add(coverageArtifact); } - PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext); env.registerAction( new TestRunnerAction( ruleContext.getActionOwner(), @@ -369,7 +368,10 @@ private TestParams createTestAction(int shards) { run, config, ruleContext.getWorkspaceName(), - shExecutable)); + (!isUsingTestWrapperInsteadOfTestSetupScript || + executionSettings.needsShell(isExecutedOnWindows)) + ? ShToolchain.getPathOrError(ruleContext) + : null)); results.add(cacheStatus); } } 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 ead826f758260b..2a04dd453873a7 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 @@ -94,7 +94,7 @@ public class TestRunnerAction extends AbstractAction private final Artifact cacheStatus; private final PathFragment testWarningsPath; private final PathFragment unusedRunfilesLogPath; - private final PathFragment shExecutable; + @Nullable private final PathFragment shExecutable; private final PathFragment splitLogsPath; private final PathFragment splitLogsDir; private final PathFragment undeclaredOutputsDir; @@ -165,7 +165,7 @@ private static ImmutableList list(Artifact... artifacts) { int runNumber, BuildConfiguration configuration, String workspaceName, - PathFragment shExecutable) { + @Nullable PathFragment shExecutable) { super( owner, /*tools=*/ ImmutableList.of(), @@ -827,7 +827,8 @@ public Artifact getCollectCoverageScript() { return collectCoverageScript; } - public PathFragment getShExecutable() { + @Nullable + public PathFragment getShExecutableMaybe() { return shExecutable; } 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 29c35c08d38127..3277d11cb93c2c 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 @@ -138,4 +138,19 @@ public Artifact getInputManifest() { public Artifact getInstrumentedFileManifest() { return instrumentedFileManifest; } + + public boolean needsShell(boolean executedOnWindows) { + RunUnder r = getRunUnder(); + if (r == null) { + return false; + } + String command = r.getCommand(); + if (command == null) { + return false; + } + // --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("\\")); + } } 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 2eb8c081ebd60a..f96f517537aa00 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.exec; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; @@ -313,7 +314,10 @@ private Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResult resu // configuration, not the machine Bazel happens to run on. Change this to something like: // testAction.getConfiguration().getExecOS() == OS.WINDOWS if (OS.getCurrent() == OS.WINDOWS && !action.isUsingTestWrapperInsteadOfTestSetupScript()) { - args.add(action.getShExecutable().getPathString()); + // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the + // native test wrapper. Something clearly went wrong. + Preconditions.checkNotNull(action.getShExecutableMaybe(), "%s", action); + args.add(action.getShExecutableMaybe().getPathString()); args.add("-c"); args.add("$0 $*"); } diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java index c8462544a6bbfc..637be71375577a 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.exec; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; @@ -165,7 +166,10 @@ public static ImmutableList getArgs(TestRunnerAction testAction) throws final boolean useTestWrapper = testAction.isUsingTestWrapperInsteadOfTestSetupScript(); if (executedOnWindows && !useTestWrapper) { - args.add(testAction.getShExecutable().getPathString()); + // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the + // native test wrapper. Something clearly went wrong. + Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction); + args.add(testAction.getShExecutableMaybe().getPathString()); args.add("-c"); args.add("$0 \"$@\""); } @@ -200,20 +204,17 @@ private static void addRunUnderArgs( if (execSettings.getRunUnderExecutable() != null) { args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getCallablePathString()); } else { - String command = execSettings.getRunUnder().getCommand(); - // --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. - boolean needsShell = - !command.contains("/") && (!executedOnWindows || !command.contains("\\")); - if (needsShell) { - String shellExecutable = testAction.getShExecutable().getPathString(); + if (execSettings.needsShell(executedOnWindows)) { + // 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(); args.add(shellExecutable); args.add("-c"); args.add("\"$@\""); args.add(shellExecutable); // Sets $0. } - args.add(command); + args.add(execSettings.getRunUnder().getCommand()); } args.addAll(testAction.getExecutionSettings().getRunUnder().getOptions()); }