Bjacklyn/fix keyerror when cloning override environment with boolean override GitHub

[bjacklyn]

Added a test to demonstrate the issue in python3.

$ python3 runtest.py SCons/EnvironmentTests.py
1/1 (100.00%) /usr/bin/python3 SCons/EnvironmentTests.py
...................................................................................E.............................................
======================================================================
ERROR: test_Clone_with_boolean_override (__main__.OverrideEnvironmentTestCase)
Test cloning an instance of OverrideEnvironment
----------------------------------------------------------------------
Traceback (most recent call last):
  File "SCons/EnvironmentTests.py", line 3897, in test_Clone_with_boolean_override
    clone_env2 = env2.Clone(BOOLEAN=False)
  File "/home/brandon/fw/releng/scons/SCons/Environment.py", line 1450, in Clone
    clone = copy.copy(self)
  File "/usr/lib/python3.5/copy.py", line 105, in copy
    return _reconstruct(x, rv, 0)
  File "/usr/lib/python3.5/copy.py", line 298, in _reconstruct
    if hasattr(y, '__setstate__'):
  File "/home/brandon/fw/releng/scons/SCons/Environment.py", line 2384, in __getattr__
    attr = getattr(self.__dict__['__subject'], name)
KeyError: '__subject'

----------------------------------------------------------------------
Ran 129 tests in 1.634s

FAILED (errors=1)

And added a commit to fix the bug.

$ python3 runtest.py SCons/EnvironmentTests.py
1/1 (100.00%) /usr/bin/python3 SCons/EnvironmentTests.py
.................................................................................................................................
----------------------------------------------------------------------
Ran 129 tests in 1.599s

OK

I'm not sure why this only happens in python3, but this change looks possibly related - python/cpython@cbbec1c
They added a change to copy() to fix how booleans were handled, change is only 6 years old so might not be in python2 🙃

[mwichmann]

Curious... why do you need to clone an OverrideEnvironment? On the other hand one wonders if this has impacts elsewhere... since the copying is done on various environment classes.

[bjacklyn]

I don't think we're explicitly creating an instance of OverrideEnvironment in our code, the failure happens in a function that looks like this where one of the env vars is being overridden:

def foo(target, source, env):
    cloned_env = env.Clone(PRECOMPILED=True)

    return bar(target, source, cloned_env)

[bdbaddog]

Clone'ing an OverrideEnvironment is not supported..
(and actually makes little sense)
Cloning a throwaway lightweight specialized environment?

Can you figure out why your build is attempting such?
Likely there's an error there.

What's the stack trace in your actual code look like?

[bjacklyn]

Our build scripts have worked since 2017 with python2 and this error happens when switching to python3. We have a monorepo with many teams working in different directories. At the top-level of each team's directory we first clone the environment, or perhaps they create their own environment, or maybe they Override() something in the environment. Then that team can further partition their build scripts with further env Clone()s if needed.

I've created a toy example to show the issue:
SConstruct

env = Environment()

SConscript('some/path/SConscript', 'env')

some/path/SConscript

Import('*')

def foo(target, source, env):
    # explodes
    boolean_cloned_env = env.Clone(BOOLEAN=True)

    return baz(target, source, boolean_cloned_env)

def bar(target, source, env):
    # explodes
    boolean_cloned_env = env.Clone(BOOLEAN=False)

    return baz(target, source, boolean_cloned_env)

def baz(target, source, env):
    return None

build_foo = Builder(action = foo)
build_bar = Builder(action = bar)

cloned_env = env.Clone(BUILDERS = {'Foo' : build_foo})
override_env = cloned_env.Override({"BUILDERS": {"Foo": build_bar}})

cloned_env.Foo(["foo.out"], ["foo.in"])
override_env.Foo(["bar.out"], ["bar.in"])

The stacktrace:

$ python3.6 scripts/scons.py -C ~/scons-example/
scons: Entering directory `/home/brandon/scons-example'
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
foo(["some/path/bar.out"], ["some/path/bar.in"])
scons: *** [some/path/bar.out] KeyError : '__subject'
Traceback (most recent call last):
  File "/home/brandon/scons/scripts/../SCons/Action.py", line 1279, in execute
    result = self.execfunction(target=target, source=rsources, env=env)
  File "/home/brandon/scons-example/some/path/SConscript", line 5, in foo
    boolean_cloned_env = env.Clone(BOOLEAN=True)
  File "/home/brandon/scons/scripts/../SCons/Environment.py", line 1450, in Clone
    clone = copy.copy(self)
  File "/usr/lib/python3.6/copy.py", line 106, in copy
    return _reconstruct(x, None, *rv)
  File "/usr/lib/python3.6/copy.py", line 281, in _reconstruct
    if hasattr(y, '__setstate__'):
  File "/home/brandon/scons/scripts/../SCons/Environment.py", line 2384, in __getattr__
    attr = getattr(self.__dict__['__subject'], name)
KeyError: '__subject'
scons: building terminated because of errors.

[bdbaddog]

So if you're switching from python2 -> 3, you're also upgrading to newer SCons?

Also

override_env = cloned_env.Override({"BUILDERS": {"Foo": build_bar}})

This should be a Clone() not Override()

And Clone'ing in an Action.
Also a bad idea.

Have you tried changing the Clone's in the Action's to be Override()?

Also a Better way to do this

def foo(target, source, env):
    # explodes
    boolean_cloned_env = env.Clone(BOOLEAN=True)

    return baz(target, source, boolean_cloned_env)

def bar(target, source, env):
    # explodes
    boolean_cloned_env = env.Clone(BOOLEAN=False)

    return baz(target, source, boolean_cloned_env)

def baz(target, source, env):
    return None

Would be

def foo(target, source, env):
    return baz(target, source, env, BOOLEAN=True )

def bar(target, source, env):

    return baz(target, source, env, BOOLEAN=False)

def baz(target, source, env, BOOLEAN=MY_Default_Value_or_None):
    return None

[mwichmann]

Oh, this is deep dark magic (or, perhaps, "excess cleverness" in SCons wrapper magic). the _reconstruct function in the copy module is called with a state, because builtin pickling support generated one. The state looks like this:

{'__subject': <SCons.Script.SConscript.SConsEnvironment object at 0x7f2ac78dac10>, 'overrides': {'BUILDERS': {'Foo': <SCons.Builder.BuilderBase object at 0x7f2ac78daa00>}}}

Since that state is not None, Python checks to see if __setstate__ exists in the object so it can call it and pass it the state to set it. It doesn't, but because the code has directly called __newobj__, we're in a situation where a new OverrideEnvironment has been constructed, but its' __init__ has not been called, and now the __getattr__ is called, which checks something which would be set in the __init__. Things will eventually get fixed up, but we've already died. I think the __getattr__ indeed needs to be more defensively coded. The suggestion in the PR should work, though there might be other approaches.

The difference in behavior actually seems to be due to that now this check (from copy.copy):

reductor = getattr(x, "__reduce_ex__", None)

returns somethng:

<built-in method __reduce_ex__ of OverrideEnvironment object at 0x7f52edfa1190>

and without further digging I don't know why that is.

[bjacklyn]

I'm not upgrading scons, I'm just switching python2 -> python3 for now. Although I did check whether upgrading would fix this issue, but it is still a problem in HEAD of master. We are on 3.13 which I believe should have full support for python3.

Also the toy example was just something I made up in 10 minutes to demonstrate the issue, with the main thing I wanted to show is how it's entirely possible in userspace code to call .Clone() on an environment that was previously created from a call to .Override(), and if you do this with a boolean in python3 then this error happens.

It seems like a simple fix to just be extra careful in the getattr/setattr functions and make sure that the __subjects dict exists before trying to check key values on it.

[mwichmann]

For a while I thought this was a difference between old and new style classes, but the base of all of the environment classes inherited from object in SCons 3 and earlier so it's not that. Copying class instances is weird since it uses the pickle techniques, that means we can indeed have our __getattr__ called without calling __init__ - in fact, __init__ never gets called since the pickle information is used to populate the new instance.

In my opinion we shouldn't leave this broken, but I guess we need the maintainer's read on it. Here are a couple of additional options:

1. in the same place as the proposed patch we could do something like this:

    def __getattr__(self, name):
        try:
            attr = getattr(self.__dict__['__subject'], name)
        except KeyError:
            # might be under construction and not have populated __dict__ yet
            raise AttributeError(name)

2. We could disallow copying of this class by defining a custom copy method:

    def __copy__(self):
        from copy import Error

        raise Error("un(shallow)copyable object of type %s" % type(self))

I realize the second option wouldn't make @bjacklyn happy :( It also doesn't really meet the test of giving a more useful error - the error is more accurate but you still don't really associate it with the SConscript usage that triggers it.

[bjacklyn]

@mwichmann -- I'm curious if copy handles option 1 with grace by catching the exception and still manages to copy the object?

[mwichmann]

@mwichmann -- I'm curious if copy handles option 1 with grace by catching the exception and still manages to copy the object?

at a quick glance it seems to - that is, your "toy example" works. that may not be real-world, though...

[bjacklyn]

I assume option 1 looks like this with the -/+ from a git diff -- I'll try this on our codebase tonight and report back.

    def __getattr__(self, name):
-        attr = getattr(self.__dict__['__subject'], name)
+        try:
+            attr = getattr(self.__dict__['__subject'], name)
+        except KeyError:
+            # might be under construction and not have populated __dict__ yet
+            raise AttributeError(name)

[mwichmann]

-- I'll try this on our codebase tonight and report back.

was there any news on this?

[bjacklyn]

Hi @mwichmann sorry for the long reply, I've confirmed that your idea to raise an AttributeError instead of KeyError works. I initially didn't expect it to work since I didn't see copy() catching this exception anywhere, but looks like inside the builtin hasattr() it handles AttributeError appropriately and returns False. Nice find.