From 6cf21b8298d81fa34f583b60d3fd19a4201d8a04 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Wed, 14 Aug 2024 05:44:00 -0600 Subject: [PATCH 1/2] 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 --- CHANGES.txt | 6 +- RELEASE.txt | 2 + SCons/Variables/BoolVariable.py | 9 +- SCons/Variables/BoolVariableTests.py | 12 ++- SCons/Variables/ListVariable.py | 8 +- SCons/Variables/ListVariableTests.py | 56 +++++++++++-- SCons/Variables/PackageVariable.py | 9 +- SCons/Variables/PackageVariableTests.py | 10 +++ SCons/Variables/PathVariable.py | 10 +-- SCons/Variables/__init__.py | 13 ++- test/Variables/ListVariable.py | 104 +++++++++++++++--------- testing/framework/TestCmd.py | 5 +- testing/framework/TestCmdTests.py | 44 ++++++---- 13 files changed, 207 insertions(+), 81 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index ffebcd0cbf..d4df05c786 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -22,8 +22,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - env.Dump() now considers the "key" positional argument to be a varargs type (zero, one or many). However called, it returns a serialized result that looks like a dict. Previously, only one "key" was - accepted. and unlike the zero-args case, it was be serialized - to a string containing the value without the key. For example, if + accepted, and unlike the zero-args case, it was be serialized + to a string containing the value (without the key). For example, if "print(repr(env.Dump('CC'))" previously returned "'gcc'", it will now return "{'CC': 'gcc'}". - Add a timeout to test/ninja/default_targets.py - it's gotten stuck on @@ -40,6 +40,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - Improve wording of manpage "Functions and Environment Methods" section. Make doc function signature style more consistent - tweaks to AddOption, DefaultEnvironment and Tool,. + - Fix handling of ListVariable when supplying a quoted choice containing + a space character (issue #4585). RELEASE 4.8.0 - Sun, 07 Jul 2024 17:22:20 -0700 diff --git a/RELEASE.txt b/RELEASE.txt index 2efc001214..5b7f9f101f 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -46,6 +46,8 @@ FIXES to be avaiable. `BoolVariable`, `EnumVariable`, `ListVariable`, `PackageVariable` and `PathVariable` are added to `__all__`, so this form of import should now work again. +- Fix handling of ListVariable when supplying a quoted choice containing + a space character (issue #4585). IMPROVEMENTS ------------ diff --git a/SCons/Variables/BoolVariable.py b/SCons/Variables/BoolVariable.py index e1fe62b905..815a4b7865 100644 --- a/SCons/Variables/BoolVariable.py +++ b/SCons/Variables/BoolVariable.py @@ -32,7 +32,7 @@ ... """ -from typing import Tuple, Callable +from typing import Callable, Tuple, Union import SCons.Errors @@ -42,7 +42,7 @@ FALSE_STRINGS = ('n', 'no', 'false', 'f', '0', 'off', 'none') -def _text2bool(val: str) -> bool: +def _text2bool(val: Union[str, bool]) -> bool: """Convert boolean-like string to boolean. If *val* looks like it expresses a bool-like value, based on @@ -54,6 +54,9 @@ def _text2bool(val: str) -> bool: Raises: ValueError: if *val* cannot be converted to boolean. """ + if isinstance(val, bool): + # mainly for the subst=False case: default might be a bool + return val lval = val.lower() if lval in TRUE_STRINGS: return True @@ -63,7 +66,7 @@ def _text2bool(val: str) -> bool: raise ValueError(f"Invalid value for boolean variable: {val!r}") -def _validator(key, val, env) -> None: +def _validator(key: str, val, env) -> None: """Validate that the value of *key* in *env* is a boolean. Parameter *val* is not used in the check. diff --git a/SCons/Variables/BoolVariableTests.py b/SCons/Variables/BoolVariableTests.py index 4c9b23e805..6d950fe3a8 100644 --- a/SCons/Variables/BoolVariableTests.py +++ b/SCons/Variables/BoolVariableTests.py @@ -43,7 +43,6 @@ def test_converter(self) -> None: """Test the BoolVariable converter""" opts = SCons.Variables.Variables() opts.Add(SCons.Variables.BoolVariable('test', 'test option help', False)) - o = opts.options[0] true_values = [ @@ -76,6 +75,17 @@ def test_converter(self) -> None: with self.assertRaises(ValueError): o.converter('x') + # Synthesize the case where the variable is created with subst=False: + # Variables code won't subst before calling the converter, + # and we might have pulled a bool from the option default. + with self.subTest(): + x = o.converter(True) + assert x, f"converter returned False for {t!r}" + with self.subTest(): + x = o.converter(False) + assert not x, f"converter returned False for {t!r}" + + def test_validator(self) -> None: """Test the BoolVariable validator""" opts = SCons.Variables.Variables() diff --git a/SCons/Variables/ListVariable.py b/SCons/Variables/ListVariable.py index f795307424..0042fa8ff3 100644 --- a/SCons/Variables/ListVariable.py +++ b/SCons/Variables/ListVariable.py @@ -81,6 +81,7 @@ def __init__( if allowedElems is None: allowedElems = [] super().__init__([_f for _f in initlist if _f]) + # TODO: why sorted? don't we want to display in the order user gave? self.allowedElems = sorted(allowedElems) def __cmp__(self, other): @@ -118,7 +119,12 @@ def _converter(val, allowedElems, mapdict) -> _ListVariable: The arguments *allowedElems* and *mapdict* are non-standard for a :class:`Variables` converter: the lambda in the :func:`ListVariable` function arranges for us to be called correctly. + + Incoming values ``all`` and ``none`` are recognized and converted + into their expanded form. """ + val = val.replace('"', '') # trim any wrapping quotes + val = val.replace("'", '') if val == 'none': val = [] elif val == 'all': @@ -155,7 +161,7 @@ def _validator(key, val, env) -> None: allowedElems = env[key].allowedElems if isinstance(val, _ListVariable): # not substituted, use .data notAllowed = [v for v in val.data if v not in allowedElems] - else: # val will be a string + else: # presumably a string notAllowed = [v for v in val.split() if v not in allowedElems] if notAllowed: # Converter only synthesized 'all' and 'none', they are never diff --git a/SCons/Variables/ListVariableTests.py b/SCons/Variables/ListVariableTests.py index 9424509d14..ca5a7935b0 100644 --- a/SCons/Variables/ListVariableTests.py +++ b/SCons/Variables/ListVariableTests.py @@ -21,6 +21,8 @@ # OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +"""Test List Variables elements.""" + import copy import unittest @@ -50,18 +52,22 @@ def test_ListVariable(self) -> None: assert o.default == 'one,three' def test_converter(self) -> None: - """Test the ListVariable converter""" + """Test the ListVariable converter. + + There is now a separate validator (for a long time validation was + in the converter), but it depends on the _ListVariable instance the + converter creates, so it's easier to test them in the same function. + """ opts = SCons.Variables.Variables() opts.Add( SCons.Variables.ListVariable( 'test', 'test option help', - 'all', - ['one', 'two', 'three'], - {'ONE': 'one', 'TWO': 'two'}, + default='all', + names=['one', 'two', 'three'], + map={'ONE': 'one', 'TWO': 'two'}, ) ) - o = opts.options[0] x = o.converter('all') @@ -110,10 +116,48 @@ def test_converter(self) -> None: # invalid value should convert (no change) without error x = o.converter('no_match') assert str(x) == 'no_match', x - # ... and fail to validate + + # validator checks + + # first, the one we just set up with self.assertRaises(SCons.Errors.UserError): o.validator('test', 'no_match', {"test": x}) + # now a new option, this time with a name w/ space in it (issue #4585) + opts.Add( + SCons.Variables.ListVariable( + 'test2', + help='test2 option help', + default='two', + names=['one', 'two', 'three', 'four space'], + ) + ) + o = opts.options[1] + + def test_valid(opt, seq): + """Validate a ListVariable value. + + Call the converter manually, since we need the _ListVariable + object to pass to the validator - this would normally be done + by the Variables.Update method. + """ + x = opt.converter(seq) + self.assertIsNone(opt.validator(opt.key, x, {opt.key: x})) + + with self.subTest(): + test_valid(o, 'one') + with self.subTest(): + test_valid(o, 'one,two,three') + with self.subTest(): + test_valid(o, 'all') + with self.subTest(): + test_valid(o, 'none') + with self.subTest(): + test_valid(o, 'four space') + with self.subTest(): + test_valid(o, 'one,four space') + + def test_copy(self) -> None: """Test copying a ListVariable like an Environment would""" opts = SCons.Variables.Variables() diff --git a/SCons/Variables/PackageVariable.py b/SCons/Variables/PackageVariable.py index fc281250c7..9fa514088f 100644 --- a/SCons/Variables/PackageVariable.py +++ b/SCons/Variables/PackageVariable.py @@ -51,7 +51,7 @@ """ import os -from typing import Callable, Optional, Tuple +from typing import Callable, Optional, Tuple, Union import SCons.Errors @@ -60,13 +60,16 @@ ENABLE_STRINGS = ('1', 'yes', 'true', 'on', 'enable', 'search') DISABLE_STRINGS = ('0', 'no', 'false', 'off', 'disable') -def _converter(val): +def _converter(val: Union[str, bool]) -> Union[str, bool]: """Convert package variables. Returns True or False if one of the recognized truthy or falsy values is seen, else return the value unchanged (expected to be a path string). """ + if isinstance(val, bool): + # mainly for the subst=False case: default might be a bool + return val lval = val.lower() if lval in ENABLE_STRINGS: return True @@ -75,7 +78,7 @@ def _converter(val): return val -def _validator(key, val, env, searchfunc) -> None: +def _validator(key: str, val, env, searchfunc) -> None: """Validate package variable for valid path. Checks that if a path is given as the value, that pathname actually exists. diff --git a/SCons/Variables/PackageVariableTests.py b/SCons/Variables/PackageVariableTests.py index 0d8aa6bc81..00cf1e3aef 100644 --- a/SCons/Variables/PackageVariableTests.py +++ b/SCons/Variables/PackageVariableTests.py @@ -82,6 +82,16 @@ def test_converter(self) -> None: x = o.converter(str(False)) assert not x, "converter returned a string when given str(False)" + # Synthesize the case where the variable is created with subst=False: + # Variables code won't subst before calling the converter, + # and we might have pulled a bool from the option default. + with self.subTest(): + x = o.converter(True) + assert x, f"converter returned False for {t!r}" + with self.subTest(): + x = o.converter(False) + assert not x, f"converter returned False for {t!r}" + def test_validator(self) -> None: """Test the PackageVariable validator""" opts = SCons.Variables.Variables() diff --git a/SCons/Variables/PathVariable.py b/SCons/Variables/PathVariable.py index d5988ac47d..43904e62c7 100644 --- a/SCons/Variables/PathVariable.py +++ b/SCons/Variables/PathVariable.py @@ -93,12 +93,12 @@ class _PathVariableClass: """ @staticmethod - def PathAccept(key, val, env) -> None: + def PathAccept(key: str, val, env) -> None: """Validate path with no checking.""" return @staticmethod - def PathIsDir(key, val, env) -> None: + def PathIsDir(key: str, val, env) -> None: """Validate path is a directory.""" if os.path.isdir(val): return @@ -109,7 +109,7 @@ def PathIsDir(key, val, env) -> None: raise SCons.Errors.UserError(msg) @staticmethod - def PathIsDirCreate(key, val, env) -> None: + def PathIsDirCreate(key: str, val, env) -> None: """Validate path is a directory, creating if needed.""" if os.path.isdir(val): return @@ -123,7 +123,7 @@ def PathIsDirCreate(key, val, env) -> None: raise SCons.Errors.UserError(msg) from exc @staticmethod - def PathIsFile(key, val, env) -> None: + def PathIsFile(key: str, val, env) -> None: """Validate path is a file.""" if not os.path.isfile(val): if os.path.isdir(val): @@ -133,7 +133,7 @@ def PathIsFile(key, val, env) -> None: raise SCons.Errors.UserError(msg) @staticmethod - def PathExists(key, val, env) -> None: + def PathExists(key: str, val, env) -> None: """Validate path exists.""" if not os.path.exists(val): msg = f'Path for variable {key!r} does not exist: {val}' diff --git a/SCons/Variables/__init__.py b/SCons/Variables/__init__.py index 501505ff33..28325266fb 100644 --- a/SCons/Variables/__init__.py +++ b/SCons/Variables/__init__.py @@ -61,7 +61,10 @@ def __lt__(self, other): def __str__(self) -> str: """Provide a way to "print" a Variable object.""" - return f"({self.key!r}, {self.aliases}, {self.help!r}, {self.default!r}, {self.validator}, {self.converter})" + return ( + f"({self.key!r}, {self.aliases}, {self.help!r}, {self.default!r}, " + f"validator={self.validator}, converter={self.converter})" + ) class Variables: @@ -287,7 +290,13 @@ def Update(self, env, args: Optional[dict] = None) -> None: for option in self.options: if option.validator and option.key in values: if option.do_subst: - value = env.subst('${%s}' % option.key) + val = env[option.key] + if not SCons.Util.is_String(val): + # issue #4585: a _ListVariable should not be further + # substituted, breaks on values with spaces. + value = val + else: + value = env.subst('${%s}' % option.key) else: value = env[option.key] option.validator(option.key, value, env) diff --git a/test/Variables/ListVariable.py b/test/Variables/ListVariable.py index a322f9b8a0..d6356e822b 100644 --- a/test/Variables/ListVariable.py +++ b/test/Variables/ListVariable.py @@ -37,7 +37,7 @@ 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 @@ def check(expected): 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 @@ def check(expected): 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 @@ def check(expected): """) 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 @@ def check(expected): 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() diff --git a/testing/framework/TestCmd.py b/testing/framework/TestCmd.py index fba0b755bb..5d9aed9a15 100644 --- a/testing/framework/TestCmd.py +++ b/testing/framework/TestCmd.py @@ -1178,7 +1178,10 @@ def command_args(self, program=None, interpreter=None, arguments=None): cmd.extend([f"{k}={v}" for k, v in arguments.items()]) return cmd if isinstance(arguments, str): - arguments = arguments.split() + # Split into a list for passing to SCons - don't lose + # quotes, and don't break apart quoted substring with space. + # str split() fails on the spaces, shlex.split() on the quotes. + arguments = re.findall(r"(?:\".*?\"|\S)+", arguments) cmd.extend(arguments) return cmd diff --git a/testing/framework/TestCmdTests.py b/testing/framework/TestCmdTests.py index 4340d90b89..2a6ccebaf4 100644 --- a/testing/framework/TestCmdTests.py +++ b/testing/framework/TestCmdTests.py @@ -1,8 +1,5 @@ #!/usr/bin/env python -""" -Unit tests for the TestCmd.py module. -""" - +# # Copyright 2000-2010 Steven Knight # This module is free software, and you may redistribute it and/or modify # it under the same terms as Python itself, so long as this copyright message @@ -19,6 +16,9 @@ # AND THERE IS NO OBLIGATION WHATSOEVER TO PROVIDE MAINTENANCE, # SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. +""" +Unit tests for the TestCmd.py module. +""" import os import shutil @@ -2225,59 +2225,67 @@ def test_command_args(self) -> None: r = test.command_args('prog') expect = [run_env.workpath('prog')] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args(test.workpath('new_prog')) expect = [test.workpath('new_prog')] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args('prog', 'python') expect = ['python', run_env.workpath('prog')] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args('prog', 'python', 'arg1 arg2') expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2'] - assert r == expect, (expect, r) + self.assertEqual(expect, r) + + r = test.command_args('prog', 'python', 'arg1 arg2="quoted"') + expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2="quoted"'] + with self.subTest(): + self.assertEqual(expect, r) + + r = test.command_args('prog', 'python', 'arg1 arg2="quoted with space"') + expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2="quoted with space"'] + with self.subTest(): + self.assertEqual(expect, r) test.program_set('default_prog') default_prog = run_env.workpath('default_prog') r = test.command_args() expect = [default_prog] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args(interpreter='PYTHON') expect = ['PYTHON', default_prog] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args(interpreter='PYTHON', arguments='arg3 arg4') expect = ['PYTHON', default_prog, 'arg3', 'arg4'] - assert r == expect, (expect, r) + self.assertEqual(expect, r) # Test arguments = dict r = test.command_args(interpreter='PYTHON', arguments={'VAR1':'1'}) expect = ['PYTHON', default_prog, 'VAR1=1'] - assert r == expect, (expect, r) - + self.assertEqual(expect, r) test.interpreter_set('default_python') r = test.command_args() expect = ['default_python', default_prog] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args(arguments='arg5 arg6') expect = ['default_python', default_prog, 'arg5', 'arg6'] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args('new_prog_1') expect = [run_env.workpath('new_prog_1')] - assert r == expect, (expect, r) + self.assertEqual(expect, r) r = test.command_args(program='new_prog_2') expect = [run_env.workpath('new_prog_2')] - assert r == expect, (expect, r) - + self.assertEqual(expect, r) class start_TestCase(TestCmdTestCase): From aca5728fd680cca0182b5873b6d34fcd9f6f2821 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Tue, 27 Aug 2024 10:21:54 -0600 Subject: [PATCH 2/2] Variables testing: confirm space-containing values The previous commit introduced a change to how the framework handled arguments, which necessitated some changes in the variables code. It got too complicated, too many places would need too much logic. Just accept that the test.run(arguments="...") will never be quite like the same arguments on the CLI, and just use lists to avoid things being broken on embedded spaces - those won't be split. Many tests arleady do this, so it's nothing new. Added a comment in TestCmd to make it more clear. Signed-off-by: Mats Wichmann --- CHANGES.txt | 6 +++--- SCons/Variables/EnumVariableTests.py | 18 +++++++++++++++++- SCons/Variables/ListVariable.py | 2 -- test/Variables/EnumVariable.py | 9 ++++++--- test/Variables/PackageVariable.py | 5 +++++ test/Variables/PathVariable.py | 20 ++++++++++++++++---- testing/framework/TestCmd.py | 12 ++++++------ testing/framework/TestCmdTests.py | 8 ++++---- testing/framework/test-framework.rst | 12 ++++++++++++ 9 files changed, 69 insertions(+), 23 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index d4df05c786..cb7d672871 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -21,9 +21,9 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER From Mats Wichmann: - env.Dump() now considers the "key" positional argument to be a varargs type (zero, one or many). However called, it returns a serialized - result that looks like a dict. Previously, only one "key" was - accepted, and unlike the zero-args case, it was be serialized - to a string containing the value (without the key). For example, if + result that looks like a dict. Previously, only a single "key" was + accepted, and unlike the zero-args case, it was serialized to a + string containing just the value (without the key). For example, if "print(repr(env.Dump('CC'))" previously returned "'gcc'", it will now return "{'CC': 'gcc'}". - Add a timeout to test/ninja/default_targets.py - it's gotten stuck on diff --git a/SCons/Variables/EnumVariableTests.py b/SCons/Variables/EnumVariableTests.py index 03848f2bef..41cb396768 100644 --- a/SCons/Variables/EnumVariableTests.py +++ b/SCons/Variables/EnumVariableTests.py @@ -159,7 +159,7 @@ def valid(o, v) -> None: def invalid(o, v) -> None: with self.assertRaises( SCons.Errors.UserError, - msg=f"did not catch expected UserError for o = {o.key}, v = {v}", + msg=f"did not catch expected UserError for o = {o.key!r}, v = {v!r}", ): o.validator('X', v, {}) table = { @@ -186,6 +186,22 @@ def invalid(o, v) -> None: expected[1](opt1, v) expected[2](opt2, v) + # make sure there are no problems with space-containing entries + opts = SCons.Variables.Variables() + opts.Add( + SCons.Variables.EnumVariable( + 'test0', + help='test option help', + default='nospace', + allowed_values=['nospace', 'with space'], + map={}, + ignorecase=0, + ) + ) + opt = opts.options[0] + valid(opt, 'nospace') + valid(opt, 'with space') + if __name__ == "__main__": unittest.main() diff --git a/SCons/Variables/ListVariable.py b/SCons/Variables/ListVariable.py index 0042fa8ff3..7a0ce49c9a 100644 --- a/SCons/Variables/ListVariable.py +++ b/SCons/Variables/ListVariable.py @@ -123,8 +123,6 @@ def _converter(val, allowedElems, mapdict) -> _ListVariable: Incoming values ``all`` and ``none`` are recognized and converted into their expanded form. """ - val = val.replace('"', '') # trim any wrapping quotes - val = val.replace("'", '') if val == 'none': val = [] elif val == 'all': diff --git a/test/Variables/EnumVariable.py b/test/Variables/EnumVariable.py index a81e8060be..48f24081c9 100644 --- a/test/Variables/EnumVariable.py +++ b/test/Variables/EnumVariable.py @@ -52,8 +52,8 @@ def check(expect): allowed_values=('motif', 'gtk', 'kde'), map={}, ignorecase=1), # case insensitive EV('some', 'some option', 'xaver', - allowed_values=('xaver', 'eins'), - map={}, ignorecase=2), # make lowercase + allowed_values=('xaver', 'eins', 'zwei wörter'), + map={}, ignorecase=2), # case lowering ) _ = DefaultEnvironment(tools=[]) @@ -89,11 +89,14 @@ def check(expect): test.run(arguments='guilib=IrGeNdwas', stderr=expect_stderr, status=2) expect_stderr = """ -scons: *** Invalid value for enum variable 'some': 'irgendwas'. Valid values are: ('xaver', 'eins') +scons: *** Invalid value for enum variable 'some': 'irgendwas'. Valid values are: ('xaver', 'eins', 'zwei wörter') """ + test.python_file_line(SConstruct_path, 20) test.run(arguments='some=IrGeNdwas', stderr=expect_stderr, status=2) +test.run(arguments=['some=zwei Wörter']) +check(['no', 'gtk', 'zwei wörter']) # case-lowering converter + test.pass_test() # Local Variables: diff --git a/test/Variables/PackageVariable.py b/test/Variables/PackageVariable.py index 64e0fa878c..e87164cab5 100644 --- a/test/Variables/PackageVariable.py +++ b/test/Variables/PackageVariable.py @@ -70,6 +70,11 @@ def check(expect): test.run(arguments=['x11=%s' % test.workpath()]) check([test.workpath()]) +space_subdir = test.workpath('space subdir') +test.subdir(space_subdir) +test.run(arguments=[f'x11={space_subdir}']) +check([space_subdir]) + expect_stderr = """ scons: *** Path does not exist for variable 'x11': '/non/existing/path/' """ + test.python_file_line(SConstruct_path, 13) diff --git a/test/Variables/PathVariable.py b/test/Variables/PathVariable.py index effbd49dc2..af9efd3281 100644 --- a/test/Variables/PathVariable.py +++ b/test/Variables/PathVariable.py @@ -106,12 +106,19 @@ def check(expect): default_file = test.workpath('default_file') default_subdir = test.workpath('default_subdir') + existing_subdir = test.workpath('existing_subdir') test.subdir(existing_subdir) existing_file = test.workpath('existing_file') test.write(existing_file, "existing_file\n") +space_subdir = test.workpath('space subdir') +test.subdir(space_subdir) + +space_file = test.workpath('space file') +test.write(space_file, "space_file\n") + non_existing_subdir = test.workpath('non_existing_subdir') non_existing_file = test.workpath('non_existing_file') @@ -135,17 +142,22 @@ def check(expect): test.run(arguments=['X=%s' % existing_file]) check([existing_file]) -test.run(arguments=['X=%s' % non_existing_file]) -check([non_existing_file]) - test.run(arguments=['X=%s' % existing_subdir]) check([existing_subdir]) +test.run(arguments=['X=%s' % space_file]) +check([space_file]) + +test.run(arguments=['X=%s' % space_subdir]) +check([space_subdir]) + test.run(arguments=['X=%s' % non_existing_subdir]) check([non_existing_subdir]) +test.must_not_exist(non_existing_subdir) +test.run(arguments=['X=%s' % non_existing_file]) +check([non_existing_file]) test.must_not_exist(non_existing_file) -test.must_not_exist(non_existing_subdir) test.write(SConstruct_path, """\ opts = Variables(args=ARGUMENTS) diff --git a/testing/framework/TestCmd.py b/testing/framework/TestCmd.py index 5d9aed9a15..616297ad45 100644 --- a/testing/framework/TestCmd.py +++ b/testing/framework/TestCmd.py @@ -1023,7 +1023,7 @@ def __init__( interpreter=None, workdir=None, subdir=None, - verbose=None, + verbose: int = -1, match=None, match_stdout=None, match_stderr=None, @@ -1039,7 +1039,7 @@ def __init__( self.description_set(description) self.program_set(program) self.interpreter_set(interpreter) - if verbose is None: + if verbose == -1: try: verbose = max(0, int(os.environ.get('TESTCMD_VERBOSE', 0))) except ValueError: @@ -1178,10 +1178,10 @@ def command_args(self, program=None, interpreter=None, arguments=None): cmd.extend([f"{k}={v}" for k, v in arguments.items()]) return cmd if isinstance(arguments, str): - # Split into a list for passing to SCons - don't lose - # quotes, and don't break apart quoted substring with space. - # str split() fails on the spaces, shlex.split() on the quotes. - arguments = re.findall(r"(?:\".*?\"|\S)+", arguments) + # Split into a list for passing to SCons. This *will* + # break if the string has embedded spaces as part of a substing - + # use a # list to pass those to avoid the problem. + arguments = arguments.split() cmd.extend(arguments) return cmd diff --git a/testing/framework/TestCmdTests.py b/testing/framework/TestCmdTests.py index 2a6ccebaf4..a2d941d611 100644 --- a/testing/framework/TestCmdTests.py +++ b/testing/framework/TestCmdTests.py @@ -2239,13 +2239,13 @@ def test_command_args(self) -> None: expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2'] self.assertEqual(expect, r) - r = test.command_args('prog', 'python', 'arg1 arg2="quoted"') - expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2="quoted"'] + r = test.command_args('prog', 'python', 'arg1 arg2=value') + expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2=value'] with self.subTest(): self.assertEqual(expect, r) - r = test.command_args('prog', 'python', 'arg1 arg2="quoted with space"') - expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2="quoted with space"'] + r = test.command_args('prog', 'python', ['arg1', 'arg2=with space']) + expect = ['python', run_env.workpath('prog'), 'arg1', 'arg2=with space'] with self.subTest(): self.assertEqual(expect, r) diff --git a/testing/framework/test-framework.rst b/testing/framework/test-framework.rst index 39ec6e3752..1a07923e91 100644 --- a/testing/framework/test-framework.rst +++ b/testing/framework/test-framework.rst @@ -621,6 +621,18 @@ or:: test.must_match(..., match=TestSCons.match_re, ...) +Often you want to supply arguments to SCons when it is invoked +to run a test, which you can do using an *arguments* parameter:: + + test.run(arguments="-O -v FOO=BAR") + +One caveat here: the way the parameter is processed is unavoidably +different from typing on the command line - if you need it not to +be split on spaces, pre-split it yourself, and pass the list, like:: + + test.run(arguments=["-f", "SConstruct2", "FOO=Two Words"]) + + Avoiding tests based on tool existence ======================================