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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

@gbedwell I think that, if on Windows, execute_external happens to be True and bash is available, it is more likely that existing lit tests (assuming LLVM is representative) will pass with some random version of bash we find on PATH than with cmd.exe. They are pretty different shells, after all. So, I'm hesitant to prefer cmd over bash if we can find it.

It's crummy that it's sensitive to the environment, of course. Maybe by forcing lit to use cmd.exe and letting the tests fail, more users will realize that they should change their configuration to use execute_external=False.

I guess I don't have a strong opinion one way or the other.

In https://reviews.llvm.org/D52831#1255972, @zturner wrote:

> In https://reviews.llvm.org/D52831#1255897, @gbedwell wrote:
>
> >   execute_external && isWindows -> 'cmd.exe'
> >
>
>
> My suggestion is to make this case assert.  It should not ever happen.  `execute_external` should imply not-windows


That'd be nice, but we'd have to disable or delete the tests that explicitly exercise `execute_external` on Windows.


https://reviews.llvm.org/D52831