[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 added a comment.

In https://reviews.llvm.org/D52831#1255765, @rnk wrote:

> Running cmd.exe is probably even less supported than running bash. If someone is accidentally using execute_external=True, I'd prefer it if they kept using bash and not cmd. So, I'd stick with the first patch.


I'm fine either way, but my curiosity has gotten the better of me now :)

I only happen to have a usable bash installation on my Windows PC because I have git for windows installed and on my path.  Someone who does everything via SVN on Windows likely won't have bash.  https://llvm.org/docs/GettingStartedVS.html#software only lists Python and Gnuwin32, which doesn't appear to include a shell, so there's no hard requirement to have bash listed anywhere.

If cmd really is a second class citizen, my concern is that the most likely deciding factor is going to be whether you happen to have git installed or not, hence my preference for the approach that essentially simplifies things to:

  execute_external && isWindows -> 'cmd.exe'
  execute_external && !isWindows -> getBashPath() or '/bin/sh'

so at least it's less reliant on external factors such as my preferred SCM system.

We used to warn about not finding bash which was removed because it was causing the Windows bots to always show up yellow (see https://reviews.llvm.org/rL228221).  If the case of accidentally using execute_external=True is a concern I wonder if there's a benefit to resurrecting the warning only in the case of

  execute_external && not litConfig.getBashPath()

as long as it could be done in such a way that those two above tests which explicitly force the external shell wouldn't cause the warning to be triggered every time and thus reintroduce the noise on the bots (I have to confess I've not yet looked to see how this would work).

> We should push all the remaining ones to invoke sh -c ... directly for the shell they need, and make the internal lit shell the default lit shell implementation everywhere. It'll simplify lit code and make tests more portable.

Agreed.  This is definitely the way forward.  I wish I could find enough time to work on this :-\


https://reviews.llvm.org/D52831