Make the test runner understand Windows command shell execution.

[zturner]

Summary

Currently, the test runner makes the assumption that it will run commands through /bin/sh. This is obviously not true on Windows, so this patch abstracts this logic out somewhat.

Instead of having the caller build the command string himself, the caller will now pass in argument list of the form [[a, b], [c, d], ...] which will get converted into a string of the form a b; c d or a b && c d, depending on the platform. By specifying shell=True to Popen, this will pass the command to /bin/sh on Nix platforms, and cmd on Windows platforms.

Windows test runner is still broken after this change, but it gets further.

Will test on Linux soon and make appropriate fixes as necessary.

Untested on Mac / FreeBSD, would appreciate some help there if possible.

[zturner]

Fixed a bug which was causing failures on Linux. All tests pass on Linux. Still untested on FreeBSD / MacOSX.

[tfiala]

Probably worth a run on MacOSX. It might take me until the weekend to hit
that, but I'll get to it if nobody else beats me to it.

[tfiala]

Giving this a run on MacOSX now.

[tfiala]

This change looks like it fails all the dsym tests on MacOSX.

Looking into it now.

Errors look like this (at least one of the error patterns):

Traceback (most recent call last):
  File "/Users/tfiala/lldb/svn/lldb/test/lldbtest.py", line 358, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/tfiala/lldb/svn/lldb/test/types/TestIntegerTypesExpr.py", line 111, in test_unsigned_int_type_with_dsym
    self.build_and_run_expr('unsigned_int.cpp', set(['unsigned', 'int']))
  File "/Users/tfiala/lldb/svn/lldb/test/types/AbstractBase.py", line 66, in build_and_run_expr
    self.build_and_run_with_source_atoms_expr(source, atoms, expr=True, dsym=dsym, bc=bc, qd=qd)
  File "/Users/tfiala/lldb/svn/lldb/test/types/AbstractBase.py", line 75, in build_and_run_with_source_atoms_expr
    self.buildDsym(dictionary=d)
  File "/Users/tfiala/lldb/svn/lldb/test/lldbtest.py", line 1297, in buildDsym
    if not module.buildDsym(self, architecture, compiler, dictionary, clean):
  File "/Users/tfiala/lldb/svn/lldb/test/plugins/builder_darwin.py", line 14, in buildDsym
    commands.append("make", "MAKE_DSYM=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary))
TypeError: append() takes exactly one argument (5 given)
Config=x86_64-clang

[tfiala]

This is the only tweak needed to your initial patch. Below is the complete diff on test/plugins/builder_darwin.py:

Index: test/plugins/builder_darwin.py
===================================================================
--- test/plugins/builder_darwin.py	(revision 213654)
+++ test/plugins/builder_darwin.py	(working copy)
@@ -7,19 +7,13 @@
 
 def buildDsym(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
     """Build the binaries with dsym debug info."""
+    commands = []
+
     if clean:
-        lldbtest.system(["/bin/sh", "-c",
-                         "make clean" + getCmdLine(dictionary)
-                         + "; make MAKE_DSYM=YES"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
-    else:
-        lldbtest.system(["/bin/sh", "-c",
-                         "make MAKE_DSYM=YES"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
+        commands.append(["make", "clean", getCmdLine(dictionary)])
+    commands.append(["make", "MAKE_DSYM=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
 
+    lldbtest.system(commands, sender=sender)
+
     # True signifies that we can handle building dsym.
     return True

(The incremental diff would be the square brackets added on the final commands.append() call.)

LGTM if you take care of adding those square brackets.

[zturner]

Closed by commit rL213669 (authored by @zturner).

test/functionalities/data-formatter/data-formatter-skip-summary/TestDataFormatterSkipSummary.py

def data_formatter_commands(self):
                                'Level 4',
                                'm_child2 = Level 4',
                                '}'])
 
         # Bad debugging info on SnowLeopard gcc (Apple Inc. build 5666).
         # Skip the following tests if the condition is met.
         if self.getCompiler().endswith('gcc') and not self.getCompiler().endswith('llvm-gcc'):
            import re
-           gcc_version_output = system([lldbutil.which(self.getCompiler()), "-v"])[1]
+           gcc_version_output = system([[lldbutil.which(self.getCompiler()), "-v"]])[1]
            #print "my output:", gcc_version_output
            for line in gcc_version_output.split(os.linesep):
                m = re.search('\(Apple Inc\. build ([0-9]+)\)', line)
                #print "line:", line
                if m:
                    gcc_build = int(m.group(1))
                    #print "gcc build:", gcc_build
                    if gcc_build >= 5666:

test/functionalities/single-quote-in-filename-to-lldb/TestSingleQuoteInFilename.py

def classCleanup(cls):
             os.remove(cls.myexe)
         except:
             pass
 
     def test_lldb_invocation_with_single_quote_in_filename(self):
         """Test that 'lldb my_file_name' works where my_file_name is a string with a single quote char in it."""
         import pexpect
         self.buildDefault()
-        system(["/bin/sh", "-c", "cp a.out \"%s\"" % self.myexe])
+        system([["cp", "a.out", "\"%s\"" % self.myexe]])
 
         # The default lldb prompt.
         prompt = "(lldb) "
 
         # So that the child gets torn down after the test.
         self.child = pexpect.spawn('%s %s "%s"' % (self.lldbHere, self.lldbOption, self.myexe))
         child = self.child
         child.setecho(True)

test/lang/cpp/virtual/TestVirtual.py

def setUp(self):
         # Find the line number to break for main.cpp.
         self.line = line_number('main.cpp', '// Set first breakpoint here.')
 
     def virtual_madness_test(self):
         """Test that variable expressions with basic types are evaluated correctly."""
 
         # First, capture the golden output emitted by the oracle, i.e., the
         # series of printf statements.
-        go = system("./a.out", sender=self)[0]
+        go = system([["./a.out"]], sender=self)[0]
         # This golden list contains a list of "my_expr = 'value' pairs extracted
         # from the golden output.
         gl = []
 
         # Scan the golden output line by line, looking for the pattern:
         #
         #     my_expr = 'value'
         #

test/lldbtest.py

def __exit__(self, type, value, tb):
         if self.trace:
             print >> sys.stderr, self.getvalue()
         if self.session:
             print >> self.session, self.getvalue()
         self.close()
 
 # From 2.7's subprocess.check_output() convenience function.
 # Return a tuple (stdoutdata, stderrdata).
-def system(*popenargs, **kwargs):
+def system(commands, **kwargs):
     r"""Run an os command with arguments and return its output as a byte string.
 
     If the exit code was non-zero it raises a CalledProcessError.  The
     CalledProcessError object will have the return code in the returncode
     attribute and output in the output attribute.
 
     The arguments are the same as for the Popen constructor.  Example:
 
     >>> check_output(["ls", "-l", "/dev/null"])
     'crw-rw-rw- 1 root root 1, 3 Oct 18  2007 /dev/null\n'
 
     The stdout argument is not allowed as it is used internally.
     To capture standard error in the result, use stderr=STDOUT.
 
     >>> check_output(["/bin/sh", "-c",
     ...               "ls -l non_existent_file ; exit 0"],
     ...              stderr=STDOUT)
     'ls: non_existent_file: No such file or directory\n'
     """
 
     # Assign the sender object to variable 'test' and remove it from kwargs.
     test = kwargs.pop('sender', None)
 
+    separator = None
+    separator = " && " if os.name == "nt" else "; "
+    # [['make', 'clean', 'foo'], ['make', 'foo']] -> ['make clean foo', 'make foo']
+    commandList = [' '.join(x) for x in commands]
+    # ['make clean foo', 'make foo'] -> 'make clean foo; make foo'
+    shellCommand = separator.join(commandList)
+
     if 'stdout' in kwargs:
         raise ValueError('stdout argument not allowed, it will be overridden.')
-    process = Popen(stdout=PIPE, stderr=PIPE, *popenargs, **kwargs)
+    if 'shell' in kwargs and kwargs['shell']==False:
+        raise ValueError('shell=False not allowed')
+    process = Popen(shellCommand, stdout=PIPE, stderr=PIPE, shell=True, **kwargs)
     pid = process.pid
     output, error = process.communicate()
     retcode = process.poll()
 
     with recording(test, traceAlways) as sbuf:
-        if isinstance(popenargs, types.StringTypes):
-            args = [popenargs]
-        else:
-            args = list(popenargs)
         print >> sbuf
-        print >> sbuf, "os command:", args
+        print >> sbuf, "os command:", shellCommand
         print >> sbuf, "with pid:", pid
         print >> sbuf, "stdout:", output
         print >> sbuf, "stderr:", error
         print >> sbuf, "retcode:", retcode
         print >> sbuf
 
     if retcode:
         cmd = kwargs.get("args")
         if cmd is None:
-            cmd = popenargs[0]
+            cmd = shellCommand
         raise CalledProcessError(retcode, cmd)
     return (output, error)
 
 def getsource_if_available(obj):
     """
     Return the text of the source code for an object if available.  Otherwise,
     a print representation is returned.
     """
class Base(unittest2.TestCase):
 
     def tearDown(self):
         """Fixture for unittest test case teardown."""
         #import traceback
         #traceback.print_stack()
 
         # This is for the case of directly spawning 'lldb' and interacting with it
         # using pexpect.
-        import pexpect
         if self.child and self.child.isalive():
+            import pexpect
             with recording(self, traceAlways) as sbuf:
                 print >> sbuf, "tearing down the child process...."
             try:
                 if self.child_in_script_interpreter:
                     self.child.sendline('quit()')
                     self.child.expect_exact(self.child_prompt)
                 self.child.sendline('settings set interpreter.prompt-on-quit false')
                 self.child.sendline('quit')
def dumpSessionInfo(self):
                     print >> self.session, traceback
 
         testMethod = getattr(self, self._testMethodName)
         if getattr(testMethod, "__benchmarks_test__", False):
             benchmarks = True
         else:
             benchmarks = False
 
-        # This records the compiler version used for the test.
-        system([self.getCompiler(), "-v"], sender=self)
-
         dname = os.path.join(os.environ["LLDB_TEST"],
                              os.environ["LLDB_SESSION_DIRNAME"])
         if not os.path.isdir(dname):
             os.mkdir(dname)
         fname = os.path.join(dname, "%s-%s-%s-%s.log" % (prefix, self.getArchitecture(), "_".join(self.getCompiler().split('/')), self.id()))
         with open(fname, "w") as f:
             import datetime
             print >> f, "Session info generated @", datetime.datetime.now().ctime()
class Base(unittest2.TestCase):
     def getCompilerVersion(self):
         """ Returns a string that represents the compiler version.
             Supports: llvm, clang.
         """
         from lldbutil import which
         version = 'unknown'
 
         compiler = self.getCompiler()
-        version_output = system([which(compiler), "-v"])[1]
+        version_output = system([[which(compiler), "-v"]])[1]
         for line in version_output.split(os.linesep):
             m = re.search('version ([0-9\.]+)', line)
             if m:
                 version = m.group(1)
         return version
 
     def isIntelCompiler(self):
         """ Returns true if using an Intel (ICC) compiler, false otherwise. """

test/plugins/builder_base.py

 def getArchFlag():
     """Returns the flag required to specify the arch"""
     compiler = getCompiler()
     if compiler is None:
       return ""
     elif "gcc" in compiler:
       archflag = "-m"
     elif "clang" in compiler:
-      archflag = "-arch "
+      archflag = "-arch"
     else:
       archflag = None
 
-    return (" ARCHFLAG=" + archflag) if archflag else ""
+    return ("ARCHFLAG=" + archflag) if archflag else ""
 
 def getMake():
     """Returns the name for GNU make"""
     if platform.system() == "FreeBSD":
-      return "gmake "
+      return "gmake"
     else:
-      return "make "
+      return "make"
 
 def getArchSpec(architecture):
     """
     Helper function to return the key-value string to specify the architecture
     used for the make system.
     """
     arch = architecture if architecture else None
     if not arch and "ARCH" in os.environ:
         arch = os.environ["ARCH"]
 
-    # Note the leading space character.
-    return (" ARCH=" + arch) if arch else ""
+    return ("ARCH=" + arch) if arch else ""
 
 def getCCSpec(compiler):
     """
     Helper function to return the key-value string to specify the compiler
     used for the make system.
     """
     cc = compiler if compiler else None
     if not cc and "CC" in os.environ:
         cc = os.environ["CC"]
-
-    # Note the leading space character.
-    return (" CC=" + cc) if cc else ""
+    if cc:
+        return "CC=\"%s\"" % cc
+    else:
+        return ""
 
 def getCmdLine(d):
     """
     Helper function to return a properly formatted command line argument(s)
     string used for the make system.
     """
 
     # If d is None or an empty mapping, just return an empty string.
     if not d:
         return ""
 
     cmdline = " ".join(["%s='%s'" % (k, v) for k, v in d.items()])
 
-    # Note the leading space character.
-    return " " + cmdline
+    return cmdline
 
 
 def buildDefault(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
     """Build the binaries the default way."""
+    commands = []
     if clean:
-        lldbtest.system(["/bin/sh", "-c",
-                         getMake() + "clean" + getCmdLine(dictionary) + ";"
-                         + getMake()
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
-    else:
-        lldbtest.system(["/bin/sh", "-c",
-                         getMake() + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
+        commands.append([getMake(), "clean", getCmdLine(dictionary)])
+    commands.append([getMake(), getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
+
+    lldbtest.system(commands, sender=sender)
 
     # True signifies that we can handle building default.
     return True
 
 def buildDwarf(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
     """Build the binaries with dwarf debug info."""
+    commands = []
     if clean:
-        lldbtest.system(["/bin/sh", "-c",
-                         getMake() + "clean" + getCmdLine(dictionary)
-                         + ";" + getMake() + "MAKE_DSYM=NO"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
-    else:
-        lldbtest.system(["/bin/sh", "-c",
-                         getMake() + "MAKE_DSYM=NO"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
+        commands.append([getMake(), "clean", getCmdLine(dictionary)])
+    commands.append([getMake(), "MAKE_DSYM=NO", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary)])
 
+    lldbtest.system(commands, sender=sender)
     # True signifies that we can handle building dwarf.
     return True
 
 def cleanup(sender=None, dictionary=None):
     """Perform a platform-specific cleanup after the test."""
     #import traceback
     #traceback.print_stack()
+    commands = []
     if os.path.isfile("Makefile"):
-        lldbtest.system(["/bin/sh", "-c",
-                         getMake() + "clean" + getCmdLine(dictionary)],
-                        sender=sender)
+        commands.append([getMake(), "clean", getCmdLine(dictionary)])
 
+    lldbtest.system(commands, sender=sender)
     # True signifies that we can handle cleanup.
     return True

test/plugins/builder_darwin.py

 import os
 import lldbtest
 
 from builder_base import *
 
 #print "Hello, darwin plugin!"
 
 def buildDsym(sender=None, architecture=None, compiler=None, dictionary=None, clean=True):
     """Build the binaries with dsym debug info."""
+    commands = []
+
     if clean:
-        lldbtest.system(["/bin/sh", "-c",
-                         "make clean" + getCmdLine(dictionary)
-                         + "; make MAKE_DSYM=YES"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
-    else:
-        lldbtest.system(["/bin/sh", "-c",
-                         "make MAKE_DSYM=YES"
-                         + getArchSpec(architecture) + getCCSpec(compiler)
-                         + getCmdLine(dictionary)],
-                        sender=sender)
+        commands.append(["make", "clean", getCmdLine(dictionary)])
+    commands.append("make", "MAKE_DSYM=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary))
+
+    lldbtest.system(commands, sender=sender)
 
     # True signifies that we can handle building dsym.
     return True

test/source-manager/TestSourceManager.py

def move_and_then_display_source(self):
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Move main.c to hidden/main.c.
         main_c = "main.c"
         main_c_hidden = os.path.join("hidden", main_c)
         os.rename(main_c, main_c_hidden)
 
         if self.TraceOn():
-            system(["ls"])
-            system(["ls", "hidden"])
+            system([["ls"]])
+            system([["ls", "hidden"]])
 
         # Restore main.c after the test.
         self.addTearDownHook(lambda: os.rename(main_c_hidden, main_c))
 
         # Set target.source-map settings.
         self.runCmd("settings set target.source-map %s %s" % (os.getcwd(), os.path.join(os.getcwd(), "hidden")))
         # And verify that the settings work.
         self.expect("settings show target.source-map",