Merge branch 'master' into issue/unique-delete-existing

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
@@ -42,6 +42,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       DefaultEnvironment and Tool,.
     - Fix a problem with AppendUnique and PrependUnique where a value could
       be erroneously removed due to a substring match.
+    - 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

@@ -48,6 +48,8 @@ FIXES
   so this form of import should now work again.
 - Fix a problem with AppendUnique and PrependUnique where a value could
   be erroneously removed due to a substring match.
+- 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/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()

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,6 +119,9 @@ 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.
     """
     if val == 'none':
         val = []
@@ -155,7 +159,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)

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: