[PATCH] D52831: [lit] Only return a found bash executable on Windows if it can understand Windows paths

[Greg Bedwell via Phabricator via llvm-commits]

gbedwell updated this revision to Diff 168190.
gbedwell added a comment.

Thanks both for reviewing.  Given that it appears that there doesn't, as I'd worried, appear to be some special hidden use-case for bash as an external shell on Windows, but is more of a historical artifact, I'm going to propose an alternative approach.  To keep the code all in the same review, and as it's in a completely separate file, I've just added it to this review in the same patch instead.  To be clear, I'm talking about committing only one of these files, not both.

On balance, I prefer the approach of removing the code path for bash on Windows (i.e. the TestRunner.py change), which reduces our matrix to essentially what @zturner said above and generally seems a bit cleaner.


https://reviews.llvm.org/D52831

Files:
  utils/lit/lit/LitConfig.py
  utils/lit/lit/TestRunner.py


Index: utils/lit/lit/TestRunner.py
===================================================================
--- utils/lit/lit/TestRunner.py
+++ utils/lit/lit/TestRunner.py
@@ -1073,18 +1073,14 @@
     return out, err, exitCode, timeoutInfo
 
 def executeScript(test, litConfig, tmpBase, commands, cwd):
-    bashPath = litConfig.getBashPath()
-    isWin32CMDEXE = (litConfig.isWindows and not bashPath)
     script = tmpBase + '.script'
-    if isWin32CMDEXE:
+    if litConfig.isWindows:
         script += '.bat'
 
     # Write script file
     mode = 'w'
-    if litConfig.isWindows and not isWin32CMDEXE:
-      mode += 'b'  # Avoid CRLFs when writing bash scripts.
     f = open(script, mode)
-    if isWin32CMDEXE:
+    if litConfig.isWindows:
         for i, ln in enumerate(commands):
             commands[i] = re.sub(kPdbgRegex, "echo '\\1' > nul && ", ln)
         if litConfig.echo_all_commands:
@@ -1103,9 +1099,11 @@
     f.write('\n')
     f.close()
 
-    if isWin32CMDEXE:
+    if litConfig.isWindows:
         command = ['cmd','/c', script]
     else:
+        bashPath = litConfig.getBashPath()
+
         if bashPath:
             command = [bashPath, script]
         else:
Index: utils/lit/lit/LitConfig.py
===================================================================
--- utils/lit/lit/LitConfig.py
+++ utils/lit/lit/LitConfig.py
@@ -120,6 +120,17 @@
         if self.bashPath is None:
             self.bashPath = ''
 
+        # Check whether the found version of bash is able to cope with paths in
+        # the host path format. If not, don't return it as it can't be used to
+        # run scripts. For example, WSL's bash.exe requires '/mnt/c/foo' rather
+        # than 'C:\\foo' or 'C:/foo'.
+        if self.isWindows and self.bashPath:
+            command = [self.bashPath, '-c',
+                       '[[ -f "%s" ]]' % self.bashPath.replace('\\', '\\\\')]
+            _, _, exitCode = lit.util.executeCommand(command)
+            if exitCode:
+                self.bashPath = ''
+
         return self.bashPath
 
     def getToolsPath(self, dir, paths, tools):


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D52831.168190.patch
Type: text/x-patch
Size: 2102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181003/a04503bb/attachment.bin>