Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix missing code contents that should be included in signature of a f… #3256

Merged
merged 1 commit into from
Dec 25, 2018

Conversation

ztessler
Copy link
Contributor

…unction action

The code of a python function that is used as a build action is inspected and included in the sconsign signature so the target can be rebuilt if the function changes. However, scons currently does not notice if a constant value in the expression of a list comprehension is changed. This PR fixes that.

In this example:

def test(source, target, env):
    xs = [2*x for x in range(5)]
    with open(str(target[0])) as out:
        out.write(str(xs))

env = Environment()
env.Command(
    source=None,
    target='out.txt',
    action=test)

changing the 2 in the list comprehension in test() would (incorrectly) not cause a rebuild.

The existing code that computes contents of a function (in _code_contents()) strips off the first item from the co_consts tuple. In a function, this first item is the docstring (or None if there is no docstring). List comprehension code is not included directly in a function's co_code, but appears as an object in the co_consts tuple, and is processed on a later recursive invocation of _code_contents(). List comprehensions also have their own co_consts tuple, though theirs do not have a docstring (or a None placeholder). Removing the first object from their co_consts effectively ignores a real constant from the code. This PR changes the logic to only remove values from the co_consts tuple that equal the docstring, if any.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

@@ -258,8 +258,7 @@ def _code_contents(code, docstring=None):
# function. Note that we have to call _object_contents on each
# constants because the code object of nested functions can
# show-up among the constants.

z = [_object_contents(cc) for cc in code.co_consts[1:]]
z = [_object_contents(cc) for cc in code.co_consts if cc != docstring]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the docstring always passed in?
Barring that if not passed in, can we grab it in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callpath to _code_contents() for a function (or method or callable class instance) goes through _function_contents(), which does pass along the docstring. The only other docstring-equipped object that comes to mind are modules - not sure if they could act as a function for the purposes of FunctionAction.

When the docstring doesn't exist, this code strips out any None's from the signature. I can't imagine any circumstances where that would be a problem though, particularly as that is similar to the existing behavior.

@bdbaddog
Copy link
Contributor

Good catch.
Looks good. I added one comment regarding docstring. Please take a look

@bdbaddog bdbaddog merged commit 2e684c2 into SCons:master Dec 25, 2018
@ztessler ztessler deleted the listcomps-in-funcaction branch December 26, 2018 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants