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

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

I really like the idea, but I think we could make a better distinction between hosting on Windows and targetting Windows.
Why don't we use bash.exe if we find it? Are there any problems with using a bash implementation on Windows?
How about letting the user provide a bash.exe path, if they want to have an external shell to execute these tests?


================
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)
----------------
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`?

================
Comment at: test/lit.common.cfg:118
@@ +117,3 @@
+else:
+  config.available_features.add('posix')
+
----------------
Please name them host-windows and host-posix, as these features are about the host.
If you're running Android tests on a Windows host, for example, you won't run tests which REQUIRES: host-posix. But you should be able to run tests requiring posix functions, since they will run on a posix target.


================
Comment at: test/ubsan/TestCases/TypeCheck/misaligned.cpp:2
@@ +1,3 @@
+// FIXME: This test currently fails on Windows because we use the MSVC linker,
+// which throws away DWARF debug info.
+// REQUIRES: posix
----------------
Shouldn't the SourceLocs always be there? Or are there cases when they aren't? If so, how easy/hard is it to fix clang instead of "hiding" the problem?

================
Comment at: test/ubsan/TestCases/TypeCheck/misaligned.cpp:3
@@ +2,3 @@
+// which throws away DWARF debug info.
+// REQUIRES: posix
+//
----------------
>From the comment above, it seems this should be an `XFAIL` when we have *both* `host-windows` and "target Windows", no?


http://reviews.llvm.org/D11960