[lit] Don't use bash on Windows. Pipeing stdout to Filecheck doesn't work.

[mpividori]

Summary

Hi,
I disable usage of bash for tests on Windows. Instead, lets use the cmd prompt.
When working on tests for libFuzzer on Windows, I found some problems when pipeing stdout to Filecheck. So, there is a problem between bash and Filecheck.

For example, this lit test fails when using bash on Windows:

CHECK: COVERAGE
RUN: not LLVMFuzzer-NullDerefTest -print_coverage=1 2>&1 | FileCheck %s

But it works fine when using the command prompt.

If we redirect the stdout to a file and then read that file, it works fine, for example:

CHECK: COVERAGE
RUN: not LLVMFuzzer-NullDerefTest -print_coverage=1 2>&1 > sometmpfile
RUN: cat sometmpfile | FileCheck %s

If we try with tee, like:

CHECK: COVERAGE
RUN: not LLVMFuzzer-NullDerefTest -print_coverage=1 2>&1 | tee sometmpfile
RUN: cat sometmpfile | FileCheck %s

It works fine.

So the problem is when pipeing to Filecheck on bash on Windows.

I can see that FileCheck is using a MemoryBuffer to read stdin.
If I modify the function getMemoryBufferForStream in lib/Support/MemoryBuffer.cpp to add a sleep(1) before reading, the tests works fine.
So, I guess there is some problem with read() returning 0 in an unexpected situation, before reading all the stdin.

Unfortunately I don't have the time to continue debugging to see what is the reason for the problem. So, I disable bash for tests on Windows.

I think this commit makes sense, since bash is not installed by default on Windows, and is not required for lit tests on: http://llvm.org/docs/GettingStartedVS.html#requirements
We only require Python and GnuWin32. So, I think is preferable to use the same terminal for all tests in all Windows machines, instead of having different situations depending on having bash installed or not.

[zturner]

I support this patch because it reduces unpredictableness in the test suite. I see no advantage to having the test suite silently behave differently depending on whether you have a piece of 3rd party software installed on Windows.

If we decide to revisit this in the future, we should make sure that we only support the official Windows Ubuntu (not git Bash), and we should have a buildbot which explicitly tests using this configuration. I've seen issues crop up in the past that would only occur on my machine but not another person's machine (or vice versa), and it's very likely this could have been the cause.

Give it some time to see what other people say, but my vote is for lgtm.

utils/lit/lit/TestRunner.py

575

I would rather initialize the variable isWin32CMDEXE first, and then for bashPath, say bashPath = None if isWin32CMDEXE else litConfig.getBashPath().

This makes sure that some logic later on will not break if it remembers to only check one variable or the other.

[mpividori]

This commit is not strictly necessary after the diff https://reviews.llvm.org/D29529 . I mean, I do not need these changes since I am using the python pipe line.
Would you like to add this changes anyway?

utils/lit/lit/TestRunner.py

for i,result in enumerate(results):
         if litConfig.maxIndividualTestTime > 0:
             out += 'error: command reached timeout: %s\n' % (
                 str(result.timeoutReached),)
 
     return out, err, exitCode, timeoutInfo
 
 def executeScript(test, litConfig, tmpBase, commands, cwd):
     bashPath = litConfig.getBashPath()
-    isWin32CMDEXE = (litConfig.isWindows and not bashPath)
+    isWin32CMDEXE = litConfig.isWindows
     script = tmpBase + '.script'
     if isWin32CMDEXE:
         script += '.bat'
 
     # Write script file
     mode = 'w'
     if litConfig.isWindows and not isWin32CMDEXE:
       mode += 'b'  # Avoid CRLFs when writing bash scripts.