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>

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

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
 ------------

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.

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()

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

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()

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.

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()

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}'

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)