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

[Zachary Turner via llvm-commits]

I personally think we should never use bash on Windows (wsl being the
exception but that won’t identify as Windows anyway). There’s value in
consistency and in my ideal world the lit shell would be feature rich
enough that we wouldn’t have to use bash *anywhere*. In any case right now
the configuration matrix is Windows (lit shell) and non Windows (bash). I
don’t think we should grow this by supporting Windows (bash)
On Wed, Oct 3, 2018 at 8:17 AM Greg Bedwell via Phabricator <
reviews at reviews.llvm.org> wrote:

> gbedwell created this revision.
> gbedwell added reviewers: ddunbar, zturner, rnk.
> Herald added a subscriber: delcypher.
>
> I've been putting up with a couple of failing tests locally for a little
> while now and finally made the time to investigate the problem:
>
>   Failing Tests (2):
>       lit :: shtest-format.py
>       lit :: shtest-run-at-line.py
>
> This seemed to be happening on my PC only.  Digging deeper, I found this
> error nested deep in the logs:
>
>   ******************** TEST 'shtest-format :: external_shell/fail.txt'
> FAILED ********************
>   Script:
>   --
>   : 'RUN: at line 3';   echo "line 1: failed test output on stdout"
>   : 'RUN: at line 4';   echo "line 2: failed test output on stdout"
>   : 'RUN: at line 5';   cat "does-not-exist"
>   --
>   Exit Code: 127
>
>   Command Output (stderr):
>   --
>   /bin/bash:
> E:workupstream-llvmbuild-vs2015-native-ninjautilslittestsInputsshtest-formatexternal_shellOutputfail.txt.script:
> No such file or directory
>
>   --
>
> It turns out that the problem was that WSL installation had placed
> bash.exe in C:\windows\system32 which was now used in preference to my
> other version of bash ("C:\Program Files\Git\usr\bin\bash.exe").  The
> primary difference being that unlike git's bash.exe, WSL's bash.exe can't
> cope with being invoked with
>
>   bash.exe c:\\foo\\script.sh
>
> and would instead need to be invoked with:
>
>   bash.exe /mnt/c/foo/script.sh
>
> I've worked around the problem by just changing the order of paths in my
> environment variable so that git bash gets preference, but to save someone
> else running into the same thing, here's an attempt at fixing it.  It's not
> the most elegant solution, but seems the least intrusive in terms of
> changing current behaviour.
>
> An obvious question is whether we should ever be using bash on Windows,
> considering everything seems to pass without it anyway.  Is there a
> specific use-case for that combination or could we just force it to use
> 'cmd /c' in TestRunner::executeScript when on Windows for all cases
> nowadays?
>
>
> https://reviews.llvm.org/D52831
>
> Files:
>   utils/lit/lit/LitConfig.py
>
>
> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181003/de2da8a7/attachment.html>