From a4ab466c6df5bd3187e78c36bfa8f68e1fb7659e Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Thu, 9 Mar 2023 12:05:47 -0700 Subject: [PATCH 1/2] 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 --- CHANGES.txt | 9 +++++++++ SCons/Environment.py | 19 ++++++++++++++++++- SCons/EnvironmentTests.py | 23 ++++++++++++++--------- test/ParseConfig.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 2d8e53bab6..8bb3c21f7f 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 9140d27264..bd94832a12 100644 --- a/SCons/Environment.py +++ b/SCons/Environment.py @@ -1043,6 +1043,12 @@ def MergeFlags(self, args, unique=True) -> None: 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 @@ def MergeFlags(self, args, unique=True) -> None: 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 @@ def MergeFlags(self, args, unique=True) -> None: for v in orig[::-1]: if v not in t: t.insert(0, v) + self[key] = t @@ -1419,7 +1436,7 @@ def Append(self, **kw): 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 fb089b1a58..81143d5c08 100644 --- a/SCons/EnvironmentTests.py +++ b/SCons/EnvironmentTests.py @@ -902,6 +902,11 @@ def test_MergeFlags(self): 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 __call__(self, command): ('-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 test_parse_flags(self): 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 test_clone_parse_flags(self): 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 @@ def test_parse_flags(self): 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 efb3a75f1f..c53e25798a 100644 --- a/test/ParseConfig.py +++ b/test/ParseConfig.py @@ -33,6 +33,7 @@ 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 @@ 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(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 @@ ['-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: From e58b9c382ddf00c48027a9610589a59062ecce70 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Thu, 9 Mar 2023 12:50:43 -0700 Subject: [PATCH 2/2] ParseConfig test fix Signed-off-by: Mats Wichmann --- test/ParseConfig.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ParseConfig.py b/test/ParseConfig.py index c53e25798a..109a3a76ce 100644 --- a/test/ParseConfig.py +++ b/test/ParseConfig.py @@ -122,7 +122,7 @@ """ stdout4 = """\ -['_REENTRANT', 'TOOLS_ENABLED'] +['TOOLS_ENABLED', '_REENTRANT'] ['pulse'] ['-pthread'] ['-pthread']