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: #4319

Closes #8161.

PiperOrigin-RevId: 246275913

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;
@@ -366,7 +367,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;
@@ -168,7 +169,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 \"$@\"");
     }
@@ -203,20 +207,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());
   }

src/test/py/bazel/test_wrapper_test.py

@@ -210,32 +210,29 @@ def _CreateMockWorkspace(self):
         ],
         executable=True)
 
-  def _AssertPassingTest(self, flag):
+  def _AssertPassingTest(self, flags):
     exit_code, _, stderr = self.RunBazel([
         'test',
         '//foo:passing_test',
         '-t-',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
-  def _AssertFailingTest(self, flag):
+  def _AssertFailingTest(self, flags):
     exit_code, _, stderr = self.RunBazel([
         'test',
         '//foo:failing_test',
         '-t-',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 3, stderr)
 
-  def _AssertPrintingTest(self, flag):
+  def _AssertPrintingTest(self, flags):
     exit_code, stdout, stderr = self.RunBazel([
         'test',
         '//foo:printing_test',
         '-t-',
         '--test_output=all',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
     lorem = False
     for line in stderr + stdout:
@@ -268,16 +265,15 @@ def _AssertPrintingTest(self, flag):
     if not user:
       self._FailWithOutput(stderr + stdout)
 
-  def _AssertRunfiles(self, flag):
+  def _AssertRunfiles(self, flags):
     exit_code, stdout, stderr = self.RunBazel([
         'test',
         '//foo:runfiles_test',
         '-t-',
         '--test_output=all',
         # Ensure Bazel does not create a runfiles tree.
         '--enable_runfiles=no',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
     mf = mf_only = rf_dir = None
     for line in stderr + stdout:
@@ -303,14 +299,13 @@ def _AssertRunfiles(self, flag):
     if not os.path.isdir(rf_dir):
       self._FailWithOutput(stderr + stdout)
 
-  def _AssertShardedTest(self, flag):
+  def _AssertShardedTest(self, flags):
     exit_code, stdout, stderr = self.RunBazel([
         'test',
         '//foo:sharded_test',
         '-t-',
         '--test_output=all',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
     status = None
     index_lines = []
@@ -328,14 +323,13 @@ def _AssertShardedTest(self, flag):
     if sorted(index_lines) != ['INDEX=0 TOTAL=2', 'INDEX=1 TOTAL=2']:
       self._FailWithOutput(stderr + stdout)
 
-  def _AssertUnexportsEnvvars(self, flag):
+  def _AssertUnexportsEnvvars(self, flags):
     exit_code, stdout, stderr = self.RunBazel([
         'test',
         '//foo:unexported_test',
         '-t-',
         '--test_output=all',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
     good = bad = None
     for line in stderr + stdout:
@@ -346,7 +340,7 @@ def _AssertUnexportsEnvvars(self, flag):
     if not good or bad:
       self._FailWithOutput(stderr + stdout)
 
-  def _AssertTestArgs(self, flag):
+  def _AssertTestArgs(self, flags):
     exit_code, bazel_bin, stderr = self.RunBazel(['info', 'bazel-bin'])
     self.AssertExitCode(exit_code, 0, stderr)
     bazel_bin = bazel_bin[0]
@@ -364,8 +358,7 @@ def _AssertTestArgs(self, flag):
         '--test_arg="x y"',
         '--test_arg=""',
         '--test_arg=qux',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
     actual = []
@@ -393,7 +386,7 @@ def _AssertTestArgs(self, flag):
         ],
         actual)
 
-  def _AssertUndeclaredOutputs(self, flag):
+  def _AssertUndeclaredOutputs(self, flags):
     exit_code, bazel_testlogs, stderr = self.RunBazel(
         ['info', 'bazel-testlogs'])
     self.AssertExitCode(exit_code, 0, stderr)
@@ -404,8 +397,7 @@ def _AssertUndeclaredOutputs(self, flag):
         '//foo:undecl_test',
         '-t-',
         '--test_output=errors',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
     undecl_zip = os.path.join(bazel_testlogs, 'foo', 'undecl_test',
@@ -450,7 +442,7 @@ def _AssertUndeclaredOutputs(self, flag):
     if mf_content[1] != 'out2/data2.dat\t16\tapplication/octet-stream':
       self._FailWithOutput(mf_content)
 
-  def _AssertUndeclaredOutputsAnnotations(self, flag):
+  def _AssertUndeclaredOutputsAnnotations(self, flags):
     exit_code, bazel_testlogs, stderr = self.RunBazel(
         ['info', 'bazel-testlogs'])
     self.AssertExitCode(exit_code, 0, stderr)
@@ -461,8 +453,7 @@ def _AssertUndeclaredOutputsAnnotations(self, flag):
         '//foo:annot_test',
         '-t-',
         '--test_output=errors',
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
     undecl_annot = os.path.join(bazel_testlogs, 'foo', 'annot_test',
@@ -474,7 +465,7 @@ def _AssertUndeclaredOutputsAnnotations(self, flag):
 
     self.assertListEqual(annot_content, ['Hello aHello c'])
 
-  def _AssertXmlGeneration(self, flag, split_xml=False):
+  def _AssertXmlGeneration(self, flags, split_xml=False):
     exit_code, bazel_testlogs, stderr = self.RunBazel(
         ['info', 'bazel-testlogs'])
     self.AssertExitCode(exit_code, 0, stderr)
@@ -486,8 +477,7 @@ def _AssertXmlGeneration(self, flag, split_xml=False):
         '-t-',
         '--test_output=errors',
         '--%sexperimental_split_xml_generation' % ('' if split_xml else 'no'),
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
     test_xml = os.path.join(bazel_testlogs, 'foo', 'xml_test', 'test.xml')
@@ -524,7 +514,7 @@ def _AssertXmlGeneration(self, flag, split_xml=False):
         'stderr_line_2' not in stderr_lines[1]):
       self._FailWithOutput(xml_contents)
 
-  def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False):
+  def _AssertXmlGeneratedByTestIsRetained(self, flags, split_xml=False):
     exit_code, bazel_testlogs, stderr = self.RunBazel(
         ['info', 'bazel-testlogs'])
     self.AssertExitCode(exit_code, 0, stderr)
@@ -536,8 +526,7 @@ def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False):
         '-t-',
         '--test_output=errors',
         '--%sexperimental_split_xml_generation' % ('' if split_xml else 'no'),
-        flag,
-    ])
+    ] + flags)
     self.AssertExitCode(exit_code, 0, stderr)
 
     test_xml = os.path.join(bazel_testlogs, 'foo', 'xml2_test', 'test.xml')
@@ -566,6 +555,7 @@ def testRunningTestFromExternalRepo(self):
             'test',
             '-t-',
             '--incompatible_windows_native_test_wrapper',
+            '--shell_executable=',
             '--test_output=errors',
             '--verbose_failures',
             flag,
@@ -577,37 +567,39 @@ def testRunningTestFromExternalRepo(self):
 
   def testTestExecutionWithTestSetupSh(self):
     self._CreateMockWorkspace()
-    flag = '--noincompatible_windows_native_test_wrapper'
-    self._AssertPassingTest(flag)
-    self._AssertFailingTest(flag)
-    self._AssertPrintingTest(flag)
-    self._AssertRunfiles(flag)
-    self._AssertShardedTest(flag)
-    self._AssertUnexportsEnvvars(flag)
-    self._AssertTestArgs(flag)
-    self._AssertUndeclaredOutputs(flag)
-    self._AssertUndeclaredOutputsAnnotations(flag)
-    self._AssertXmlGeneration(flag, split_xml=False)
-    self._AssertXmlGeneration(flag, split_xml=True)
-    self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False)
-    self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True)
+    flags = ['--noincompatible_windows_native_test_wrapper']
+    self._AssertPassingTest(flags)
+    self._AssertFailingTest(flags)
+    self._AssertPrintingTest(flags)
+    self._AssertRunfiles(flags)
+    self._AssertShardedTest(flags)
+    self._AssertUnexportsEnvvars(flags)
+    self._AssertTestArgs(flags)
+    self._AssertUndeclaredOutputs(flags)
+    self._AssertUndeclaredOutputsAnnotations(flags)
+    self._AssertXmlGeneration(flags, split_xml=False)
+    self._AssertXmlGeneration(flags, split_xml=True)
+    self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=False)
+    self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=True)
 
   def testTestExecutionWithTestWrapperExe(self):
     self._CreateMockWorkspace()
-    flag = '--incompatible_windows_native_test_wrapper'
-    self._AssertPassingTest(flag)
-    self._AssertFailingTest(flag)
-    self._AssertPrintingTest(flag)
-    self._AssertRunfiles(flag)
-    self._AssertShardedTest(flag)
-    self._AssertUnexportsEnvvars(flag)
-    self._AssertTestArgs(flag)
-    self._AssertUndeclaredOutputs(flag)
-    self._AssertUndeclaredOutputsAnnotations(flag)
-    self._AssertXmlGeneration(flag, split_xml=False)
-    self._AssertXmlGeneration(flag, split_xml=True)
-    self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False)
-    self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True)
+    flags = [
+        '--incompatible_windows_native_test_wrapper', '--shell_executable='
+    ]
+    self._AssertPassingTest(flags)
+    self._AssertFailingTest(flags)
+    self._AssertPrintingTest(flags)
+    self._AssertRunfiles(flags)
+    self._AssertShardedTest(flags)
+    self._AssertUnexportsEnvvars(flags)
+    self._AssertTestArgs(flags)
+    self._AssertUndeclaredOutputs(flags)
+    self._AssertUndeclaredOutputsAnnotations(flags)
+    self._AssertXmlGeneration(flags, split_xml=False)
+    self._AssertXmlGeneration(flags, split_xml=True)
+    self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=False)
+    self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=True)
 
 
 if __name__ == '__main__':