Merge pull request #4322 from mwichmann/bug/ParseConfig-CPPDEFINES

Fix problem when MergeFlags adds to existing CPPDEFINES

4 files changed, 72 insertions, 10 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 2d8e53bab..8bb3c21f7 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -13,6 +13,15 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Remove the redundant `wheel` dependency from `pyproject.toml`,
       as it is added automatically by the setuptools PEP517 backend.
 
+  From Mats Wichmann
+    -  Fix a problem (#4321) in 4.5.0/4.5.1 where ParseConfig could cause an
+       exception in MergeFlags when the result would be to add preprocessor
+       defines to existing CPPDEFINES. The following code illustrates the
+       circumstances that could trigger this:
+          env=Environment(CPPDEFINES=['a'])
+          env.Append(CPPDEFINES=['b'])
+          env.MergeFlags({'CPPDEFINES': 'c'})
+
 
 RELEASE 4.5.1 -  Mon, 06 Mar 2023 14:08:29 -0700
 
diff --git a/SCons/Environment.py b/SCons/Environment.py
index 9140d2726..bd94832a1 100644
--- a/SCons/Environment.py
+++ b/SCons/Environment.py
@@ -1043,6 +1043,12 @@ class SubstitutionEnvironment:
         flags distributed into appropriate construction variables.
         See :meth:`ParseFlags`.
 
+        As a side effect, if *unique* is true, a new object is created
+        for each modified construction variable by the loop at the end.
+        This is silently expected by the :meth:`Override` *parse_flags*
+        functionality, which does not want to share the list (or whatever)
+        with the environment being overridden.
+
         Args:
             args: flags to merge
             unique: merge flags rather than appending (default: True).
@@ -1077,6 +1083,16 @@ class SubstitutionEnvironment:
                     try:
                         orig = orig + value
                     except (KeyError, TypeError):
+                        # If CPPDEFINES is a deque, adding value (a list)
+                        # results in TypeError, so we handle that case here.
+                        # Just in case we got called from Override, make
+                        # sure we make a copy, because we don't go through
+                        # the cleanup loops at the end of the outer for loop,
+                        # which implicitly gives us a new object.
+                        if isinstance(orig, deque):
+                            self[key] = self[key].copy()
+                            self.AppendUnique(CPPDEFINES=value, delete_existing=True)
+                            continue
                         try:
                             add_to_orig = orig.append
                         except AttributeError:
@@ -1095,6 +1111,7 @@ class SubstitutionEnvironment:
                 for v in orig[::-1]:
                     if v not in t:
                         t.insert(0, v)
+
             self[key] = t
 
 
@@ -1419,7 +1436,7 @@ class Base(SubstitutionEnvironment):
             if key == 'CPPDEFINES':
                 _add_cppdefines(self._dict, val)
                 continue
-                
+
             try:
                 orig = self._dict[key]
             except KeyError:
diff --git a/SCons/EnvironmentTests.py b/SCons/EnvironmentTests.py
index fb089b1a5..81143d5c0 100644
--- a/SCons/EnvironmentTests.py
+++ b/SCons/EnvironmentTests.py
@@ -902,6 +902,11 @@ sys.exit(0)
         assert env['A'] == ['aaa'], env['A']
         assert env['B'] == ['b', 'bb', 'bbb'], env['B']
 
+        # issue #4231: CPPDEFINES can be a deque, tripped up merge logic
+        env = Environment(CPPDEFINES=deque(['aaa', 'bbb']))
+        env.MergeFlags({'CPPDEFINES': 'ccc'})
+        self.assertEqual(env['CPPDEFINES'], deque(['aaa', 'bbb', 'ccc']))
+
         # issue #3665: if merging dict which is a compound object
         # (i.e. value can be lists, etc.), the value object should not
         # be modified. per the issue, this happened if key not in env.
@@ -2167,7 +2172,7 @@ def generate(env):
                                       ('-isystem', '/usr/include/foo2'),
                                       ('-idirafter', '/usr/include/foo3'),
                                       '+DD64'], env['CCFLAGS']
-            assert env['CPPDEFINES'] == ['FOO', ['BAR', 'value']], env['CPPDEFINES']
+            self.assertEqual(list(env['CPPDEFINES']), ['FOO', ['BAR', 'value']])
             assert env['CPPFLAGS'] == ['', '-Wp,-cpp'], env['CPPFLAGS']
             assert env['CPPPATH'] == ['string', '/usr/include/fum', 'bar'], env['CPPPATH']
             assert env['FRAMEWORKPATH'] == ['fwd1', 'fwd2', 'fwd3'], env['FRAMEWORKPATH']
@@ -3662,10 +3667,10 @@ def generate(env):
         env = Environment(tools=[], CCFLAGS=None, parse_flags = '-Y')
         assert env['CCFLAGS'] == ['-Y'], env['CCFLAGS']
 
-        env = Environment(tools=[], CPPDEFINES = 'FOO', parse_flags = '-std=c99 -X -DBAR')
+        env = Environment(tools=[], CPPDEFINES='FOO', parse_flags='-std=c99 -X -DBAR')
         assert env['CFLAGS']  == ['-std=c99'], env['CFLAGS']
         assert env['CCFLAGS'] == ['-X'], env['CCFLAGS']
-        assert env['CPPDEFINES'] == ['FOO', 'BAR'], env['CPPDEFINES']
+        self.assertEqual(list(env['CPPDEFINES']), ['FOO', 'BAR'])
 
     def test_clone_parse_flags(self):
         """Test the env.Clone() parse_flags argument"""
@@ -3687,8 +3692,7 @@ def generate(env):
         assert 'CCFLAGS' not in env
         assert env2['CCFLAGS'] == ['-X'], env2['CCFLAGS']
         assert env['CPPDEFINES'] == 'FOO', env['CPPDEFINES']
-        assert env2['CPPDEFINES'] == ['FOO','BAR'], env2['CPPDEFINES']
-
+        self.assertEqual(list(env2['CPPDEFINES']), ['FOO','BAR'])
 
 
 class OverrideEnvironmentTestCase(unittest.TestCase,TestEnvironmentFixture):
@@ -3978,15 +3982,16 @@ class OverrideEnvironmentTestCase(unittest.TestCase,TestEnvironmentFixture):
         assert env['CCFLAGS'] is None, env['CCFLAGS']
         assert env2['CCFLAGS'] == ['-Y'], env2['CCFLAGS']
 
-        env = SubstitutionEnvironment(CPPDEFINES = 'FOO')
-        env2 = env.Override({'parse_flags' : '-std=c99 -X -DBAR'})
+        env = SubstitutionEnvironment(CPPDEFINES='FOO')
+        env2 = env.Override({'parse_flags': '-std=c99 -X -DBAR'})
         assert 'CFLAGS' not in env
         assert env2['CFLAGS']  == ['-std=c99'], env2['CFLAGS']
         assert 'CCFLAGS' not in env
         assert env2['CCFLAGS'] == ['-X'], env2['CCFLAGS']
+        # make sure they are independent
+        self.assertIsNot(env['CPPDEFINES'], env2['CPPDEFINES'])
         assert env['CPPDEFINES'] == 'FOO', env['CPPDEFINES']
-        assert env2['CPPDEFINES'] == ['FOO','BAR'], env2['CPPDEFINES']
-
+        self.assertEqual(list(env2['CPPDEFINES']), ['FOO','BAR'])
 
 
 class NoSubstitutionProxyTestCase(unittest.TestCase,TestEnvironmentFixture):
diff --git a/test/ParseConfig.py b/test/ParseConfig.py
index efb3a75f1..109a3a76c 100644
--- a/test/ParseConfig.py
+++ b/test/ParseConfig.py
@@ -33,6 +33,7 @@ test = TestSCons.TestSCons()
 test_config1 = test.workpath('test-config1')
 test_config2 = test.workpath('test-config2')
 test_config3 = test.workpath('test-config3')
+test_config4 = test.workpath('test-config4')
 
 # 'abc' is supposed to be a static lib; it is included in LIBS as a
 # File node.
@@ -51,6 +52,10 @@ test.write(test_config3, """\
 print("-L foo -L lib_dir -isysroot /tmp -arch ppc -arch i386")
 """)
 
+test.write(test_config4, """\
+print("-D_REENTRANT -lpulse -pthread")
+""")
+
 test.write('SConstruct1', """
 env = Environment(CPPPATH = [], LIBPATH = [], LIBS = [],
                   CCFLAGS = '-pipe -Wall')
@@ -85,6 +90,23 @@ print([str(x) for x in env['LIBS']])
 print(env['CCFLAGS'])
 """ % locals())
 
+# issue #4321: if CPPDEFINES has been promoted to deque, adding would fail
+test.write('SConstruct4', f"""\
+env = Environment(
+    CPPDEFINES="_REENTRANT",
+    LIBS=[],
+    CCFLAGS=[],
+    LINKFLAGS=[],
+    PYTHON=r'{_python_}',
+)
+env.Append(CPPDEFINES="TOOLS_ENABLED")
+env.ParseConfig(r"$PYTHON {test_config4} --libs --cflags")
+print([str(x) for x in env['CPPDEFINES']])
+print([str(x) for x in env['LIBS']])
+print(env['CCFLAGS'])
+print(env['LINKFLAGS'])
+""")
+
 good_stdout = """\
 ['/usr/include/fum', 'bar']
 ['/usr/fax', 'foo', 'lib_dir']
@@ -99,12 +121,21 @@ stdout3 = """\
 ['-pipe', '-Wall', ('-isysroot', '/tmp'), ('-arch', 'ppc'), ('-arch', 'i386')]
 """
 
+stdout4 = """\
+['TOOLS_ENABLED', '_REENTRANT']
+['pulse']
+['-pthread']
+['-pthread']
+"""
+
 test.run(arguments = "-q -Q -f SConstruct1 .", stdout = good_stdout)
 
 test.run(arguments = "-q -Q -f SConstruct2 .", stdout = good_stdout)
 
 test.run(arguments = "-q -Q -f SConstruct3 .", stdout = stdout3)
 
+test.run(arguments = "-q -Q -f SConstruct4 .", stdout = stdout4)
+
 test.pass_test()
 
 # Local Variables: