Fix ListVariable with a space-containing value

Fix ListVariable handling of a quoted variable value containing spaces.
As a side effect of splitting the former monolithic converter/validator
for ListVariable into separate callbacks, it became possible for
subst to be called twice. The ListVariable converter produces an
instance of a _ListVariable container, and running subst on that
result ends up losing information, so avoid doing so.

While developing a test for this, it turned out the test framework
also didn't handle a quoted argument containing a space, so that
a test case passing arguments to scons via "run(arguments='...')"
could end up with scons seeing a different (broken) command line
than scons invoked with the same arguments typing to a shell prompt.
A regex is now used to more accurately split the "arguments" parameter,
and a unit test was added to the framework tests to validate.

The framework fix had a side effect - it was possible that when run
as part of the test suite, the Variables package could receive a value
still wrapped in quotes, leading to string mismatches ('"with space"'
is not equal to 'with space'), so ListVariable now strips wrapping
quote marks.

Also during testing it turned out that the earlier fix for #4241,
allowing a Variable to declare the value should not be subst'd,
introduced problems for two types which assumed they would always
be passed a string. With subst=False, they could be passed a default
value that had been specified as a bool. Fixed to not fail on that.

Fixes #4585

Signed-off-by: Mats Wichmann <mats@linux.com>

1 files changed, 65 insertions, 39 deletions

diff --git a/test/Variables/ListVariable.py b/test/Variables/ListVariable.py
index a322f9b..d6356e8 100644
--- a/test/Variables/ListVariable.py
+++ b/test/Variables/ListVariable.py
@@ -37,7 +37,7 @@ SConstruct_path = test.workpath('SConstruct')
 
 def check(expected):
     result = test.stdout().split('\n')
-    r = result[1:len(expected)+1]
+    r = result[1 : len(expected) + 1]
     assert r == expected, (r, expected)
 
 
@@ -45,17 +45,24 @@ test.write(SConstruct_path, """\
 from SCons.Variables.ListVariable import ListVariable as LV
 from SCons.Variables import ListVariable
 
-list_of_libs = Split('x11 gl qt ical')
+list_of_libs = Split('x11 gl qt ical') + ["with space"]
 
 optsfile = 'scons.variables'
 opts = Variables(optsfile, args=ARGUMENTS)
 opts.AddVariables(
-    ListVariable('shared',
-               'libraries to build as shared libraries',
-               'all',
-               names = list_of_libs,
-               map = {'GL':'gl', 'QT':'qt'}),
-    LV('listvariable', 'listvariable help', 'all', names=['l1', 'l2', 'l3'])
+    ListVariable(
+        'shared',
+        'libraries to build as shared libraries',
+        default='all',
+        names=list_of_libs,
+        map={'GL': 'gl', 'QT': 'qt'},
+    ),
+    LV(
+        'listvariable',
+        'listvariable help',
+        default='all',
+        names=['l1', 'l2', 'l3'],
+    ),
 )
 
 _ = DefaultEnvironment(tools=[])  # test speedup
@@ -70,7 +77,7 @@ if 'ical' in env['shared']:
 else:
     print('0')
 
-print(" ".join(env['shared']))
+print(",".join(env['shared']))
 
 print(env.subst('$shared'))
 # Test subst_path() because it's used in $CPPDEFINES expansions.
@@ -79,14 +86,27 @@ Default(env.Alias('dummy', None))
 """)
 
 test.run()
-check(['all', '1', 'gl ical qt x11', 'gl ical qt x11',
-       "['gl ical qt x11']"])
+check(
+    [
+        'all',
+        '1',
+        'gl,ical,qt,with space,x11',
+        'gl ical qt with space x11',
+        "['gl ical qt with space x11']",
+    ]
+)
 
-expect = "shared = 'all'"+os.linesep+"listvariable = 'all'"+os.linesep
+expect = "shared = 'all'" + os.linesep + "listvariable = 'all'" + os.linesep
 test.must_match(test.workpath('scons.variables'), expect)
-
-check(['all', '1', 'gl ical qt x11', 'gl ical qt x11',
-       "['gl ical qt x11']"])
+check(
+    [
+        'all',
+        '1',
+        'gl,ical,qt,with space,x11',
+        'gl ical qt with space x11',
+        "['gl ical qt with space x11']",
+    ]
+)
 
 test.run(arguments='shared=none')
 check(['none', '0', '', '', "['']"])
@@ -95,74 +115,80 @@ test.run(arguments='shared=')
 check(['none', '0', '', '', "['']"])
 
 test.run(arguments='shared=x11,ical')
-check(['ical,x11', '1', 'ical x11', 'ical x11',
-       "['ical x11']"])
+check(['ical,x11', '1', 'ical,x11', 'ical x11', "['ical x11']"])
 
 test.run(arguments='shared=x11,,ical,,')
-check(['ical,x11', '1', 'ical x11', 'ical x11',
-       "['ical x11']"])
+check(['ical,x11', '1', 'ical,x11', 'ical x11', "['ical x11']"])
 
 test.run(arguments='shared=GL')
 check(['gl', '0', 'gl', 'gl'])
 
 test.run(arguments='shared=QT,GL')
-check(['gl,qt', '0', 'gl qt', 'gl qt', "['gl qt']"])
+check(['gl,qt', '0', 'gl,qt', 'gl qt', "['gl qt']"])
 
+#test.run(arguments='shared="with space"')
+#check(['with space', '0', 'with space', 'with space', "['with space']"])
 
 expect_stderr = """
-scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,x11,all,none
-""" + test.python_file_line(SConstruct_path, 18)
+scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,with space,x11,all,none
+""" + test.python_file_line(SConstruct_path, 25)
 
 test.run(arguments='shared=foo', stderr=expect_stderr, status=2)
 
 # be paranoid in testing some more combinations
 
 expect_stderr = """
-scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,x11,all,none
-""" + test.python_file_line(SConstruct_path, 18)
+scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,with space,x11,all,none
+""" + test.python_file_line(SConstruct_path, 25)
 
 test.run(arguments='shared=foo,ical', stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,x11,all,none
-""" + test.python_file_line(SConstruct_path, 18)
+scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,with space,x11,all,none
+""" + test.python_file_line(SConstruct_path, 25)
 
 test.run(arguments='shared=ical,foo', stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,x11,all,none
-""" + test.python_file_line(SConstruct_path, 18)
+scons: *** Invalid value(s) for variable 'shared': 'foo'. Valid values are: gl,ical,qt,with space,x11,all,none
+""" + test.python_file_line(SConstruct_path, 25)
 
 test.run(arguments='shared=ical,foo,x11', stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Invalid value(s) for variable 'shared': 'foo,bar'. Valid values are: gl,ical,qt,x11,all,none
-""" + test.python_file_line(SConstruct_path, 18)
+scons: *** Invalid value(s) for variable 'shared': 'foo,bar'. Valid values are: gl,ical,qt,with space,x11,all,none
+""" + test.python_file_line(SConstruct_path, 25)
 
 test.run(arguments='shared=foo,x11,,,bar', stderr=expect_stderr, status=2)
 
-test.write('SConstruct', """
+test.write('SConstruct2', """\
 from SCons.Variables import ListVariable
 
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
-    ListVariable('gpib',
-               'comment',
-               ['ENET', 'GPIB'],
-               names = ['ENET', 'GPIB', 'LINUX_GPIB', 'NO_GPIB']),
-    )
+    ListVariable(
+        'gpib',
+        'comment',
+        default=['ENET', 'GPIB'],
+        names=['ENET', 'GPIB', 'LINUX_GPIB', 'NO_GPIB'],
+    ),
+)
 
 DefaultEnvironment(tools=[])  # test speedup
-env = Environment(variables=opts)
+env = Environment(tools=[], variables=opts)
 Help(opts.GenerateHelpText(env))
 
 print(env['gpib'])
 Default(env.Alias('dummy', None))
 """)
 
-test.run(stdout=test.wrap_stdout(read_str="ENET,GPIB\n", build_str="""\
+test.run(
+    arguments="-f SConstruct2",
+    stdout=test.wrap_stdout(read_str="ENET,GPIB\n",
+    build_str="""\
 scons: Nothing to be done for `dummy'.
-"""))
+""")
+)
 
 test.pass_test()