ParseConfig can fail when adding to CPPDEFINES

[mwichmann]

After the changes in CPPDEFINES handling for SCons 4.5, it is possible for ParseConfig to hit a TypeError. Here is one traceback:

TypeError: sequence index must be integer, not 'slice':
  File "/home/akien/Projects/godot/godot.git/SConstruct", line 539:
    detect.configure(env)
  File "/home/akien/Projects/godot/godot.git/./platform/linuxbsd/detect.py", line 309:
    env.ParseConfig("pkg-config libpulse --cflags --libs")
  File "/home/akien/.local/lib/python3.10/site-packages/SCons/Environment.py", line 1773:
    return function(self, self.backtick(command), unique)
  File "/home/akien/.local/lib/python3.10/site-packages/SCons/Environment.py", line 1767:
    return env.MergeFlags(cmd, unique)
  File "/home/akien/.local/lib/python3.10/site-packages/SCons/Environment.py", line 1095:
    for v in orig[::-1]:

For this condition to trigger, CPPDEFINES in that environment must have previously been converted into the internal deque type. The error actually happens in MergeFlags, which tries to do various complicated things to simulate what the Append routine would do in adding. Since Append and relatives now short-circuits that work in case the key is CPPDEFINES, it seems MergeFlags should do the same.

[akien-mga]

Attaching a minimal reproduction project, which reproduces the issue with 4.5.0 and 4.5.1:
scons-ParseConfig-CPPDEFINES-merge.zip

[mwichmann]

I'm seeing another hole here, I think - env.Override(parse_flags={some: dict}) seems to not give an independent value. Investigating - this tripped on an existing testcase after a change for the above.

[bdbaddog]

ok. Once we have a fix for both, we can roll a 4.5.2

[mwichmann]

Sigh, it looks like (without documenting it), MergeFlags always creates a new item, so my short-circuit means that doesn't happen, and so indeed that opens up a new leak-through

[mwichmann]

@akien-mga - the patch proposed in the PR ended up being different, because of the new problem that one created. Would you be able to test the change from SCons/Environment.py in your setup to make sure it still works for you?

[akien-mga]

@mwichmann I confirm that #4322 (as of e58b9c3) fixes the bug for me, applied the diff manually on top of pip-installed 4.5.1.

[mwichmann]

Not going to tackle this here, as want to keep this PR as a simple "hotfix", but don't actually see any reason why MergeFlags needs to reimplement the adding logic (it contains the comment The logic here was lifted from part of env.Append() ) - we've already proven here that having it in two places makes it more fragile. So someday... at that point, the caller should end up indicating whether the variables merged into need to be independent (Override case) or the existing environment vars can be safely updated (all other callers, AFAICT).

[mgorny]

Could you please make a new release with this fix? This is affecting at least godot on Gentoo, and quite likely dxx-rebirth (I haven't confirmed whether it's the same cause).

[bdbaddog]

Could you please make a new release with this fix? This is affecting at least godot on Gentoo, and quite likely dxx-rebirth (I haven't confirmed whether it's the same cause).

IRL issues causing delay. I'll try to get it out today sometime.

[bdbaddog]

4.5.2 released.

[mgorny]

Thank you!

[arbruijn]

There's still a deque returned by env['CPPDEFINES'] in 4.5.2 after env.Append with an existing and added list, which can break SConstruct files. The documentation seems to suggest it should result in a list (7.2.10. Appending to the End of Values: the Append Method). Is this an intended change of behavior?

Example that worked before 4.5.0 and breaks since 4.5.0:

env = Environment()
env.Append(CPPDEFINES = ['A'])
env.Append(CPPDEFINES = ['B'])
defs = env['CPPDEFINES'] + ['X']

[mwichmann]

It was an intended change. What type it is is supposed to be internal, and works consistently internally, but there's not much to be done if the API methods for amending are not used - adding an object directly using Python syntax. May have to give up on deque (it was chosen because the Prepend methods then work as cleanly as the Append methods without resorting to tricks - one of the instances in the old version, but not all, reversed, added, reversed again to Prepend).

Note that whatever that snip in the user guide says, the internal represntation was never guaranteed, and could change depending on what operations you performed. Using a different pathway of building up CPPDEFINES you could have ended up with a dict, and then your same add would fail (hypothetical example, but I ran through lots of these permutations when I expanded the tests):

emv = Environment()
env.Append(CPPDEFINES={'A': None, 'B': None})
defs = env['CPPDEFINES'] + ['X']

would take an exception

TypeError: unsupported operand type(s) for +: 'dict' and 'list'

[vLKp]

Regarding #4321 (comment) :

This is affecting at least godot on Gentoo, and quite likely dxx-rebirth (I haven't confirmed whether it's the same cause).

I think the Rebirth problem was caused by the same SCons change that broke ParseConfig, but the failure mode is different, and I think the change in a4ab466 would not help Rebirth. Rebirth had an expression of the form env.get(name, [])[:], which sought to look up a variable from the construction environment and create a copy of it. When the construction environment returned a Python list, [:] worked. Now that SCons returns a Python deque, the error reported in dxx-rebirth/dxx-rebirth#704 occurs, because deque does not understand using [:] to create a copy. I changed that expression to env.get(name, []).copy(), which works just as well on SCons 4.4. I have not tried it on SCons 4.5 yet (scons is not slotted, and I am not ready to remove SCons 4.4 yet), but I published the change in the hope that it would be useful.