Windows, testing: only look up shell if needed

TestActionBuilder now only looks up the shell (via
ShToolchain.getPathOrError) when the shell is
required.

Now, when using the Windows-native test wrapper
with --shell_toolchain="" (and without a
shell-command-looking --run_under argument) the
TestActionBuilder won't depend on Bash.

Related: bazelbuild#4319

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);
       }
     }

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<Artifact> 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;
   }
 

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("\\"));
+  }
 }

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 $*");
     }

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<String> 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());
   }