[PATCH] D11960: [windows] Always use the lit shell on Windows, even if bash is present

[Filipe Cabecinhas via llvm-commits]

filcab added inline comments.

================
Comment at: test/lit.common.cfg:15
@@ -17,1 +14,3 @@
+# Setup test format. Use bash on Unix and the lit shell on Windows.
+execute_external = (not sys.platform in ['win32'])
 config.test_format = lit.formats.ShTest(execute_external)
----------------
rnk wrote:
> filcab wrote:
> > rnk wrote:
> > > filcab wrote:
> > > > I would prefer if we could test the *target*, instead of the *host*.
> > > > I can always work around it by adding to the condition, but still. This looks fragile to me.
> > > > If we have a bash-like shell in the path, should we try it, and set `execute_external`?
> > > This is inconsistent with LLVM and Clang, and is exactly what I want to disable.
> > I'm assuming the "this is inconsistent" is referring to "target != host".
> > 
> > I'm perfectly ok with having the compiler-rt test suite be different. It *is* very different. No clang or llvm test runs on anything other than the host. All compiler-rt tests run on the target. We can't have the same rules here.
> > 
> > For my case, the characteristics between host and target are very different, but this kind of thing can bite anyone who works on having the sanitizers in external devices (like the PS4, Android, iOS, ...), since tests based on the host might be totally wrong.
> > 
> > For my case, I have to compile and run tests on Windows, right now, and forcing `execute_external` to false will make me not be able to run some tests which I should be able to run on my bash. This will also happen on the Windows->Windows case where bash is available and the tests don't need any `/somehting` command line option.
> > 
> > As I said, changing it to test for "are we targeting platform X" is easy enough, but I would really like to have a good separation between host and target on the compiler-rt tests.
> I think we should just try not to depend on too many complex bash-isms in our test suites. At some point we'll need to implement environment variables in the lit shell, which will solve a large class of issues.
> 
> Anyway, I don't see what the issue is here. Whether we have a shell has always been a property of the host that is running the tests. We support remote test execution on targets through the %run lit expansion. I don't imagine we will ever need to test if the target does or does not have bash. Therefore, I don't see the need for host-shell and target-shell features. The "shell" feature always implies that the system running the tests (host, right?) has a shell.
Yes, not depending on bash-isms is always a good thing. And yes, we'll eventually have to implement env var support in lit. Sorry for making a mess of my comment :-(
And yes, "shell" is always a feature of hosts (at least for now ;-) ).


http://reviews.llvm.org/D11960