Page MenuHomePhabricator

Make the test runner understand Windows command shell execution.

Authored by zturner on Jul 18 2014, 12:41 PM.



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.

Diff Detail


Event Timeline

zturner updated this revision to Diff 11664.Jul 18 2014, 12:41 PM
zturner retitled this revision from to Make the test runner understand Windows command shell execution..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a subscriber: Unknown Object (MLST).
zturner updated this revision to Diff 11673.Jul 18 2014, 2:56 PM

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

tfiala added a subscriber: tfiala.Jul 18 2014, 4:59 PM

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.

Giving this a run on MacOSX now.

tfiala edited edge metadata.EditedJul 22 2014, 7:48 AM

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/", line 358, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/tfiala/lldb/svn/lldb/test/types/", 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/", 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/", line 75, in build_and_run_with_source_atoms_expr
  File "/Users/tfiala/lldb/svn/lldb/test/", line 1297, in buildDsym
    if not module.buildDsym(self, architecture, compiler, dictionary, clean):
  File "/Users/tfiala/lldb/svn/lldb/test/plugins/", line 14, in buildDsym
    commands.append("make", "MAKE_DSYM=YES", getArchSpec(architecture), getCCSpec(compiler), getCmdLine(dictionary))
TypeError: append() takes exactly one argument (5 given)

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

Index: test/plugins/
--- test/plugins/	(revision 213654)
+++ test/plugins/	(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 this revision.Jul 22 2014, 9:28 AM
zturner updated this revision to Diff 11761.

Closed by commit rL213669 (authored by @zturner).