Fix commands with spaces in them (Bug: 589281 and 589285). (Anthony Roach)

3 files changed, 84 insertions, 49 deletions

diff --git a/src/engine/SCons/Action.py b/src/engine/SCons/Action.py
index 1ddecdf..2a07fce 100644
--- a/src/engine/SCons/Action.py
+++ b/src/engine/SCons/Action.py
@@ -50,6 +50,12 @@ exitvalmap = {
 
 default_ENV = None
 
+def quote(x):
+    if ' ' in x or '\t' in x:
+        return '"'+x+'"'
+    else:
+        return x
+
 if os.name == 'posix':
 
     def defaultSpawn(cmd, args, env):
@@ -57,11 +63,7 @@ if os.name == 'posix':
         if not pid:
             # Child process.
             exitval = 127
-            args = [ 'sh', '-c' ] + \
-                   [ string.join(map(lambda x: string.replace(str(x),
-                                                              ' ',
-                                                              r'\ '),
-                                     args)) ]
+            args = ['sh', '-c', string.join(map(quote, args))]
             try:
                 os.execvpe('sh', args, env)
             except OSError, e:
@@ -145,13 +147,8 @@ elif os.name == 'nt':
             return 127
         else:
             try:
-
-                a = [ cmd_interp, '/C', args[0] ]
-                for arg in args[1:]:
-                    if ' ' in arg or '\t' in arg:
-                        arg = '"' + arg + '"'
-                    a.append(arg)
-                ret = os.spawnve(os.P_WAIT, cmd_interp, a, env)
+                args = [cmd_interp, '/C', quote(string.join(map(quote, args)))]
+                ret = os.spawnve(os.P_WAIT, cmd_interp, args, env)
             except OSError, e:
                 ret = exitvalmap[e[0]]
                 sys.stderr.write("scons: %s: %s\n" % (cmd, e[1]))
diff --git a/src/engine/SCons/Util.py b/src/engine/SCons/Util.py
index 89871a3..5178956 100644
--- a/src/engine/SCons/Util.py
+++ b/src/engine/SCons/Util.py
@@ -194,64 +194,69 @@ _space_sep = re.compile(r'[\t ]+(?![^{]*})')
 
 def scons_subst_list(strSubst, globals, locals, remove=None):
     """
-    This function is similar to scons_subst(), but with
-    one important difference.  Instead of returning a single
-    string, this function returns a list of lists.
+    This function serves the same purpose as scons_subst(), except
+    this function returns the interpolated list as a list of lines, where
+    each line is a list of command line arguments. In other words:
     The first (outer) list is a list of lines, where the
     substituted stirng has been broken along newline characters.
     The inner lists are lists of command line arguments, i.e.,
     the argv array that should be passed to a spawn or exec
     function.
 
-    Also, this method can accept a list of strings as input
-    to strSubst, which explicitly denotes the command line
-    arguments.  This is useful if you want to pass in
-    command line arguments with spaces or newlines in them.
-    Otheriwise, if you just passed in a string, they would
-    get split along the spaces and newlines.
-    
-    One important thing this guy does is preserve environment
-    variables that are lists.  For instance, if you have
-    an environment variable that is a Python list (or UserList-
-    derived class) that contains path names with spaces in them,
-    then the entire path will be returned as a single argument.
-    This is the only way to know where the 'split' between arguments
-    is for executing a command line."""
+    There are a few simple rules this function follows in order to
+    determine how to parse strSubst and consruction variables into lines
+    and arguments:
+
+    1) A string is interpreted as a space delimited list of arguments.
+    2) A list is interpreted as a list of arguments. This allows arguments
+       with spaces in them to be expressed easily.
+    4) Anything that is not a list or string (e.g. a Node instance) is
+       interpreted as a single argument, and is converted to a string.
+    3) Newline (\n) characters delimit lines. The newline parsing is done
+       after all the other parsing, so it is not possible for arguments
+       (e.g. file names) to contain embedded newline characters.
+    """
 
-    def repl(m, globals=globals, locals=locals):
+    def convert(x):
+        """This function is used to convert construction variable
+        values or the value of strSubst to a string for interpolation.
+        This function follows the rules outlined in the documentaion
+        for scons_subst_list()"""
+        if x is None:
+            return ''
+        elif is_String(x):
+            return _space_sep.sub('\0', x)
+        elif is_List(x):
+            return string.join(map(to_String, x), '\0')
+        else:
+            return to_String(x)
+
+    def repl(m, globals=globals, locals=locals, convert=convert):
         key = m.group(1)
         if key[0] == '{':
             key = key[1:-1]
         try:
             e = eval(key, globals, locals)
-            if e is None:
-                s = ''
-            elif is_List(e):
-                s = string.join(map(to_String, e), '\0')
-            else:
-                s = _space_sep.sub('\0', to_String(e))
+            return convert(e)
         except NameError:
-            s = ''
-        return s
+            return ''
 
-    if is_List(strSubst):
-        # This looks like our input is a list of strings,
-        # as explained in the docstring above.  Munge
-        # it into a tokenized string by concatenating
-        # the list with nulls.
-        strSubst = string.join(strSubst, '\0')
-    else:
-        # Tokenize the original string...
-        strSubst = _space_sep.sub('\0', to_String(strSubst))
+    # Convert the argument to a string:
+    strSubst = convert(strSubst)
 
-    # Now, do the substitution
+    # Do the interpolation:
     n = 1
     while n != 0:
         strSubst, n = _cv.subn(repl, strSubst)
-    # Now parse the whole list into tokens.
+        
+    # Convert the interpolated string to a list of lines:
     listLines = string.split(strSubst, '\n')
+
+    # Remove the patterns that match the remove argument: 
     if remove:
         listLines = map(lambda x,re=remove: re.sub('', x), listLines)
+
+    # Finally split each line up into a list of arguments:
     return map(lambda x: filter(lambda y: y, string.split(x, '\0')),
                listLines)
 
diff --git a/src/engine/SCons/UtilTests.py b/src/engine/SCons/UtilTests.py
index 1f712cf..cad5670 100644
--- a/src/engine/SCons/UtilTests.py
+++ b/src/engine/SCons/UtilTests.py
@@ -136,6 +136,13 @@ class UtilTestCase(unittest.TestCase):
 
     def test_subst_list(self):
         """Testing the scons_subst_list() method..."""
+
+        class Node:
+            def __init__(self, name):
+                self.name = name
+            def __str__(self):
+                return self.name
+        
         loc = {}
         loc['TARGETS'] = PathList(map(os.path.normpath, [ "./foo/bar.exe",
                                                           "/bar/baz with spaces.obj",
@@ -147,6 +154,11 @@ class UtilTestCase(unittest.TestCase):
         loc['xxx'] = None
         loc['NEWLINE'] = 'before\nafter'
 
+        loc['DO'] = Node('do something')
+        loc['FOO'] = Node('foo.in')
+        loc['BAR'] = Node('bar with spaces.out')
+        loc['CRAZY'] = Node('crazy\nfile.in')
+
         if os.sep == '/':
             def cvt(str):
                 return str
@@ -167,6 +179,27 @@ class UtilTestCase(unittest.TestCase):
         assert cmd_list[1][0] == 'after', cmd_list[1][0]
         assert cmd_list[0][2] == cvt('../foo/ack.cbefore'), cmd_list[0][2]
 
+        cmd_list = scons_subst_list("$DO --in=$FOO --out=$BAR", loc, {})
+        assert len(cmd_list) == 1, cmd_list
+        assert len(cmd_list[0]) == 3, cmd_list
+        assert cmd_list[0][0] == 'do something', cmd_list[0][0]
+        assert cmd_list[0][1] == '--in=foo.in', cmd_list[0][1]
+        assert cmd_list[0][2] == '--out=bar with spaces.out', cmd_list[0][2]
+
+        # XXX: The newline in crazy really should be interpreted as
+        #      part of the file name, and not as delimiting a new command
+        #      line
+        #      In other words the following test fragment is illustrating
+        #      a bug in variable interpolation.
+        cmd_list = scons_subst_list("$DO --in=$CRAZY --out=$BAR", loc, {})
+        assert len(cmd_list) == 2, cmd_list
+        assert len(cmd_list[0]) == 2, cmd_list
+        assert len(cmd_list[1]) == 2, cmd_list
+        assert cmd_list[0][0] == 'do something', cmd_list[0][0]
+        assert cmd_list[0][1] == '--in=crazy', cmd_list[0][1]
+        assert cmd_list[1][0] == 'file.in', cmd_list[1][0]
+        assert cmd_list[1][1] == '--out=bar with spaces.out', cmd_list[1][1]
+        
         # Test inputting a list to scons_subst_list()
         cmd_list = scons_subst_list([ "$SOURCES$NEWLINE", "$TARGETS",
                                         "This is a test" ],