Merge pull request SCons#4554 from mwichmann/move-env-checker

Move environment var checker to SCons.Util

Author: bdbaddog

CHANGES.txt

@@ -86,15 +86,19 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       updated in 4.6). All SCons usage except unit test was already fully
       consistent with a bool.
     - When a variable is added to a Variables object, it can now be flagged
-      as "don't perform substitution" by setting the argument subst. 
-      This allows variables to contain characters which would otherwise 
+      as "don't perform substitution" by setting the argument subst.
+      This allows variables to contain characters which would otherwise
       cause expansion. Fixes #4241.
     - The test runner now recognizes the unittest module's return code of 5,
       which means no tests were run. SCons/Script/MainTests.py currently
       has no tests, so this particular error code is expected - should not
       cause runtest to give up with an "unknown error code".
     - Updated the notes about reproducible builds with SCons and the example.
     - The Clone() method now respects the variables argument (fixes #3590)
+    - is_valid_construction_var() (not part of the public API) moved from
+      SCons.Environment to SCons.Util to avoid the chance of import loops. Variables
+      and Environment both use the routine and Environment() uses a Variables()
+      object so better to move to a safer location.
 
 
 RELEASE 4.7.0 -  Sun, 17 Mar 2024 17:22:20 -0700

RELEASE.txt

@@ -107,7 +107,10 @@ DEVELOPMENT
   which means no tests were run. SCons/Script/MainTests.py currently
   has no tests, so this particular error code is expected - should not
   cause runtest to give up with an "unknown error code".
-
+- is_valid_construction_var() (not part of the public API) moved from
+  SCons.Environment to SCons.Util to avoid the chance of import loops. Variables
+  and Environment both use the routine and Environment() uses a Variables()
+  object so better to move to a safer location.
 
 Thanks to the following contributors listed below for their contributions to this release.
 ==========================================================================================

SCons/Environment.py

@@ -76,6 +76,7 @@
     to_String_for_subst,
     uniquer_hashables,
 )
+from SCons.Util.envs import is_valid_construction_var
 from SCons.Util.sctyping import ExecutorType
 
 class _Null:
@@ -510,13 +511,6 @@ def update(self, mapping) -> None:
             self.__setitem__(i, v)
 
 
-_is_valid_var = re.compile(r'[_a-zA-Z]\w*$')
-
-def is_valid_construction_var(varstr: str) -> bool:
-    """Return True if *varstr* is a legitimate construction variable."""
-    return bool(_is_valid_var.match(varstr))
-
-
 class SubstitutionEnvironment:
     """Base class for different flavors of construction environments.
 
@@ -605,7 +599,7 @@ def __setitem__(self, key, value):
             # key and we don't need to check.  If we do check, using a
             # global, pre-compiled regular expression directly is more
             # efficient than calling another function or a method.
-            if key not in self._dict and not _is_valid_var.match(key):
+            if key not in self._dict and not is_valid_construction_var(key):
                 raise UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 

SCons/EnvironmentTests.py

@@ -38,7 +38,6 @@
     NoSubstitutionProxy,
     OverrideEnvironment,
     SubstitutionEnvironment,
-    is_valid_construction_var,
 )
 from SCons.Util import CLVar
 from SCons.SConsign import current_sconsign_filename
@@ -4142,40 +4141,6 @@ def test_subst_target_source(self) -> None:
         x = proxy.subst_target_source(*args, **kw)
         assert x == ' ttt sss ', x
 
-class EnvironmentVariableTestCase(unittest.TestCase):
-
-    def test_is_valid_construction_var(self) -> None:
-        """Testing is_valid_construction_var()"""
-        r = is_valid_construction_var("_a")
-        assert r, r
-        r = is_valid_construction_var("z_")
-        assert r, r
-        r = is_valid_construction_var("X_")
-        assert r, r
-        r = is_valid_construction_var("2a")
-        assert not r, r
-        r = is_valid_construction_var("a2_")
-        assert r, r
-        r = is_valid_construction_var("/")
-        assert not r, r
-        r = is_valid_construction_var("_/")
-        assert not r, r
-        r = is_valid_construction_var("a/")
-        assert not r, r
-        r = is_valid_construction_var(".b")
-        assert not r, r
-        r = is_valid_construction_var("_.b")
-        assert not r, r
-        r = is_valid_construction_var("b1._")
-        assert not r, r
-        r = is_valid_construction_var("-b")
-        assert not r, r
-        r = is_valid_construction_var("_-b")
-        assert not r, r
-        r = is_valid_construction_var("b1-_")
-        assert not r, r
-
-
 
 if __name__ == "__main__":
     unittest.main()

SCons/Util/UtilTests.py

@@ -73,6 +73,7 @@
     to_bytes,
     to_str,
 )
+from SCons.Util.envs import is_valid_construction_var
 from SCons.Util.hashes import (
     _attempt_init_of_python_3_9_hash_object,
     _attempt_get_hash_function,
@@ -1206,6 +1207,41 @@ def test_default(self) -> None:
             assert var is False, 'var should be False, not %s' % repr(var)
 
 
+class EnvironmentVariableTestCase(unittest.TestCase):
+
+    def test_is_valid_construction_var(self) -> None:
+        """Testing is_valid_construction_var()"""
+        r = is_valid_construction_var("_a")
+        assert r, r
+        r = is_valid_construction_var("z_")
+        assert r, r
+        r = is_valid_construction_var("X_")
+        assert r, r
+        r = is_valid_construction_var("2a")
+        assert not r, r
+        r = is_valid_construction_var("a2_")
+        assert r, r
+        r = is_valid_construction_var("/")
+        assert not r, r
+        r = is_valid_construction_var("_/")
+        assert not r, r
+        r = is_valid_construction_var("a/")
+        assert not r, r
+        r = is_valid_construction_var(".b")
+        assert not r, r
+        r = is_valid_construction_var("_.b")
+        assert not r, r
+        r = is_valid_construction_var("b1._")
+        assert not r, r
+        r = is_valid_construction_var("-b")
+        assert not r, r
+        r = is_valid_construction_var("_-b")
+        assert not r, r
+        r = is_valid_construction_var("b1-_")
+        assert not r, r
+
+
+
 if __name__ == "__main__":
     unittest.main()
 

SCons/Util/__init__.py

@@ -107,6 +107,7 @@
     AppendPath,
     AddPathIfNotExists,
     AddMethod,
+    is_valid_construction_var,
 )
 from .filelock import FileLock, SConsLockFailure
 

SCons/Util/envs.py

@@ -9,6 +9,7 @@
 that don't need the specifics of the Environment class.
 """
 
+import re
 import os
 from types import MethodType, FunctionType
 from typing import Union, Callable, Optional, Any
@@ -329,6 +330,12 @@ def AddMethod(obj, function: Callable, name: Optional[str] = None) -> None:
     setattr(obj, name, method)
 
 
+_is_valid_var_re = re.compile(r'[_a-zA-Z]\w*$')
+
+def is_valid_construction_var(varstr: str) -> bool:
+    """Return True if *varstr* is a legitimate construction variable."""
+    return bool(_is_valid_var_re.match(varstr))
+
 # Local Variables:
 # tab-width:4
 # indent-tabs-mode:nil

SCons/Variables/__init__.py

@@ -28,7 +28,6 @@
 from functools import cmp_to_key
 from typing import Callable, Dict, List, Optional, Sequence, Union
 
-import SCons.Environment
 import SCons.Errors
 import SCons.Util
 import SCons.Warnings
@@ -70,13 +69,15 @@ class Variables:
       files: string or list of strings naming variable config scripts
          (default ``None``)
       args: dictionary to override values set from *files*.  (default ``None``)
-      is_global: if true, return a global singleton Variables object instead
-          of a fresh instance. Currently inoperable (default ``False``)
+      is_global: if true, return a global singleton :class:`Variables` object
+         instead of a fresh instance. Currently inoperable (default ``False``)
 
     .. versionchanged:: 4.8.0
-         The default for *is_global* changed to ``False`` (previously
-         ``True`` but it had no effect due to an implementation error).
-         *is_global* is deprecated.
+       The default for *is_global* changed to ``False`` (previously
+       ``True`` but it had no effect due to an implementation error).
+
+    .. deprecated:: 4.8.0
+       *is_global* is deprecated.
     """
 
     def __init__(
@@ -130,7 +131,7 @@ def _do_add(
             option.key = key
             # TODO: normalize to not include key in aliases. Currently breaks tests.
             option.aliases = [key,]
-        if not SCons.Environment.is_valid_construction_var(option.key):
+        if not SCons.Util.is_valid_construction_var(option.key):
             raise SCons.Errors.UserError(f"Illegal Variables key {option.key!r}")
         option.help = help
         option.default = default

bench/env.__setitem__.py

@@ -1,8 +1,7 @@
 # __COPYRIGHT__
 #
 # Benchmarks for testing various possible implementations of the
-# env.__setitem__() method(s) in the src/engine/SCons/Environment.py
-# module.
+# env.__setitem__() method(s) in the SCons/Environment.py module.
 
 from __future__ import print_function
 
@@ -25,10 +24,10 @@ def __init__(self, name, num, init, statement):
         self.name    = name
         self.statement = statement
         self.__result  = None
-        
+
     def timeit(self):
         self.__result = self.__timer.timeit(self.__num)
-        
+
     def getResult(self):
         return self.__result
 
@@ -62,8 +61,9 @@ def times(num=1000000, init='', title='Results:', **statements):
 
 import SCons.Errors
 import SCons.Environment
+import SCons.Util
 
-is_valid_construction_var = SCons.Environment.is_valid_construction_var
+is_valid_construction_var = SCons.Util.is_valid_construction_var
 global_valid_var = re.compile(r'[_a-zA-Z]\w*$')
 
 # The classes with different __setitem__() implementations that we're
@@ -80,7 +80,7 @@ def times(num=1000000, init='', title='Results:', **statements):
 #
 # The env_Original subclass contains the original implementation (which
 # actually had the is_valid_construction_var() function in SCons.Util
-# originally).
+# originally). Update: it has moved back, to avoid import loop with Variables.
 #
 # The other subclasses (except for env_Best) each contain *one*
 # significant change from the env_Original implementation.  The doc string
@@ -116,7 +116,7 @@ def __setitem__(self, key, value):
         if special:
             special(self, key, value)
         else:
-            if not SCons.Environment.is_valid_construction_var(key):
+            if not SCons.Util.is_valid_construction_var(key):
                 raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -176,7 +176,7 @@ def __setitem__(self, key, value):
         if key in self._special_set:
             self._special_set[key](self, key, value)
         else:
-            if not SCons.Environment.is_valid_construction_var(key):
+            if not SCons.Util.is_valid_construction_var(key):
                 raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -186,7 +186,7 @@ def __setitem__(self, key, value):
         if key in ('BUILDERS', 'SCANNERS', 'TARGET', 'TARGETS', 'SOURCE', 'SOURCES'):
             self._special_set[key](self, key, value)
         else:
-            if not SCons.Environment.is_valid_construction_var(key):
+            if not SCons.Util.is_valid_construction_var(key):
                 raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -196,7 +196,7 @@ def __setitem__(self, key, value):
         if key in ['BUILDERS', 'SCANNERS', 'TARGET', 'TARGETS', 'SOURCE', 'SOURCES']:
             self._special_set[key](self, key, value)
         else:
-            if not SCons.Environment.is_valid_construction_var(key):
+            if not SCons.Util.is_valid_construction_var(key):
                 raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -206,7 +206,7 @@ def __setitem__(self, key, value):
         if key in self._special_set_keys:
             self._special_set[key](self, key, value)
         else:
-            if not SCons.Environment.is_valid_construction_var(key):
+            if not SCons.Util.is_valid_construction_var(key):
                 raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -220,7 +220,7 @@ def __setitem__(self, key, value):
             try:
                 self._dict[key]
             except KeyError:
-                if not SCons.Environment.is_valid_construction_var(key):
+                if not SCons.Util.is_valid_construction_var(key):
                     raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value
 
@@ -232,7 +232,7 @@ def __setitem__(self, key, value):
             special(self, key, value)
         else:
             if key not in self._dict \
-                and not SCons.Environment.is_valid_construction_var(key):
+                and not SCons.Util.is_valid_construction_var(key):
                     raise SCons.Errors.UserError("Illegal construction variable `%s'" % key)
             self._dict[key] = value