Fix problem when MergeFlags adds to existing CPPDEFINES

MergeFlags has a post-processing step if the *unique* flag evaluates True
which loops through and removes the duplicates. This step uses slicing
(for v in orig[::-1]), which fails if the item being cleaned is a deque -
which CPPDEFINES can now be. It would also cause the deque to be
replaced with a list.  Detect this case and handle separately.

Note the same post-processing step assures each modified object will
be replaced - Override(parse_flags=xxx) silently counted on this so
it does not end up sharing variables with the overridden env. This
situation remains, and is accounted for by the patch.

Unit test and e2e tests are extended to check that MergeFlags can now
add correctly, and that Override leaves the variables independent,
not shared.

Fixes #4231

Signed-off-by: Mats Wichmann <mats@linux.com>

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..c53e25798 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 = """\
+['_REENTRANT', 'TOOLS_ENABLED']
+['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: