Closed Bug 1172800 Opened 5 years ago Closed 5 years ago

moz.build sandbox template execution refactoring

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files)

In order to enable using templates as context managers, the execution model for templates needs to change so that it allows e.g. returns and yields.
I've had these refactorings in my queue for a while, and my review comments in bug 1168607 prompted me to land it earlier rather than later, and address the issue of inspect doing I/O at the same time.
Bug 1172800 - Move moz.build template handling in a separate class

This will make subsequent refactorings a bit more intelligible.
Attachment #8617172 - Flags: review?(gps)
Bug 1172800 - Avoid using inspect.getsourcelines() and inspect.getfile()

inspect.getsourcelines() and inspect.getfile() involve I/O out of our control.
Our use of those functions, however, doesn't require all their smarts. In fact,
we only use them on function objects, for which we can just do the work
ourselves without involving inspect functions that trigger I/O.
Attachment #8617173 - Flags: review?(gps)
Bug 1172800 - Create actual functions to execute moz.build templates

The current mode of execution of templates doesn't allow them to more advanced
control flow, like returning early, returning or yielding values, because that
mode of execution is equivalent to running the code at the top level of a .py
file.

Making the templates executed through a function call, although trickier,
allows those control flows, which will be useful for template as context
managers.
Attachment #8617174 - Flags: review?(gps)
Note that I'm okay with the last patch needed more cycles of reviews ; landings parts 1 to 3 are enough to land bug 1168607. I can rebase part 4 if that happens in that order.
Assignee: nobody → mh+mozilla
Comment on attachment 8617171 [details]
MozReview Request: Bug 1172800 - Fixup after bug 991983

https://reviewboard.mozilla.org/r/10571/#review9301

Ship It!
Attachment #8617171 - Flags: review?(gps) → review+
Comment on attachment 8617172 [details]
MozReview Request: Bug 1172800 - Move moz.build template handling in a separate class

https://reviewboard.mozilla.org/r/10573/#review9303

Ship It!
Attachment #8617172 - Flags: review?(gps) → review+
Comment on attachment 8617173 [details]
MozReview Request: Bug 1172800 - Avoid using inspect.getsourcelines() and inspect.getfile()

https://reviewboard.mozilla.org/r/10575/#review9305

::: python/mozbuild/mozbuild/frontend/sandbox.py:184
(Diff revision 1)
> +            old_source = self._current_source
> +            self._current_source = source
>              exec(code, self)
> +            self._current_source = old_source

This feels like it should be a try..finally. Although I'm not sure in what cases we'd need that. Better to be safe than sorry.
Attachment #8617173 - Flags: review?(gps) → review+
Comment on attachment 8617174 [details]
MozReview Request: Bug 1172800 - Create actual functions to execute moz.build templates

https://reviewboard.mozilla.org/r/10577/#review9307

Oy.

This might be the craziest Python I've ever had to review.

But, AFAICT the implementation looks solid. It obviously works. And we have thorough test coverage of this code. So as long as the tests pass, I'm comfortable with this change.

I'm kinda curious why compile() isn't inheriting unicode_literals from the calling frame. Something tells me explicitly passing flags into that function will alleviate the need for the AST transform. But maybe I'm not grokking the C code completely (wouldn't surprise me - first time I've looked at it.)

::: python/mozbuild/mozbuild/frontend/reader.py:426
(Diff revision 1)
> -        first_op = None
> +        self.path = inspect.getfile(func)

Pretty sure you didn't mean to add this back in.

::: python/mozbuild/mozbuild/frontend/reader.py:445
(Diff revision 1)
> -        # - m is firstlineno - 1,
> +        # prepend mode underscores until we find an unused name.

s/mode/more

::: python/mozbuild/mozbuild/frontend/reader.py:490
(Diff revision 1)
> +        def __init__(self, sandbox, global_name):
> +            self._sandbox = sandbox
> +            self._global_name = global_name

Since this creates some cycles, you may want to `del` the instance of this class as soon as it is used. That or use weakref.

::: python/mozbuild/mozbuild/frontend/reader.py:494
(Diff revision 1)
> +        def visit_Str(self, node):
> +            # String nodes we got from the AST parser are str, but we want
> +            # unicode literals everywhere, so transform them.
> +            node.s = unicode(node.s)
> +            return node

This caused me to dig into CPython's source code to see how unicode_literals is handled because compile() should inherit __future__ from the calling module unless `dont_inherit` is specified. We rely on this in sandbox.py, for example.

compile() takes a string or ast as its argument. https://hg.python.org/cpython/file/ae1bc5b65e65/Python/bltinmodule.c#l466. When an ast is passed, PyAST_Compile (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/compile.c#l253) is called and the ast is compiled to code.

When a string is passed, Py_CompileStringFlags (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/pythonrun.c#l1407) is called. This calls PyParser_ASTFromString (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/pythonrun.c#l1454). This calls PyAST_FromNode (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/ast.c#l215), which sets up unicode_literals (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/ast.c#l237). Deep in the bowels of the parser, parsestr (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/ast.c#l3458) looks at the unicode flag and converts string literals to Unicode.

Anyway, the docs for `ast.parse` say the function is equivalent to `compile(source, filename, mode, ast.PyCF_ONLY_AST)`. The source confirms this: https://hg.python.org/cpython/file/ae1bc5b65e65/Lib/ast.py#l32

If dont_inherit isn't passed to compile(), we're supposed to inherit flags from the caller. However, it appears from Py_CompileStringFlags that flags don't enter the equation if PyCF_ONLY_AST is involved. So I guess the unicode conversion is happening during compilation, after the AST has been obtained (which makes obvious sense, of course). I was kinda hoping you could coax compile() into doing the unicode conversion on the returned AST. I guess not.

That being said, you do call compile() above when executing this code object. reader.py does have unicode_literals. My reading of the source code is that PyEval_MergeCompilerFlags (https://hg.python.org/cpython/file/ae1bc5b65e65/Python/ceval.c#l4156) should inherit unicode_literals from the calling frame and factor it into the compilation, causing string literals to become Unicode. Is this not actually happening?

::: python/mozbuild/mozbuild/frontend/sandbox.py:176
(Diff revision 1)
> +            exec code in self

Why did you change this from exec(code, self)? I'm actually not sure where the exec function is defined in Python 2.7, but the exec statement was dropped in Python 3, so we should retain future compatibility if possible.

::: python/mozbuild/mozbuild/frontend/reader.py:455
(Diff revision 1)
> -        # So we need to prepend with n - 1 newlines so that line numbers
> +        # The resulting of executing this code is the creation of a 'Template'

result

::: python/mozbuild/mozbuild/frontend/reader.py:458
(Diff revision 1)
> -        self._code += ''.join(lines[begin[0] - 1:])
> -        self._func = func
> +        func = types.FunctionType(
> +            compile(func_ast, self.path, 'exec'),
> +            glob,
> +            self.name,
> +            func.func_defaults,
> +            func.func_closure,
> +        )
> +        func()

Congratulations on figuring out how to use this undocumented function.
Attachment #8617174 - Flags: review?(gps) → review+
> Is this not actually happening?

Nope. If I remove the workaround this is what happens:

==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/glandium/gecko/toolkit/library/moz.build

The error was triggered on line 16 of this file:

    SHARED_LIBRARY_NAME = 'xul'

The underlying problem is an attempt to write an illegal value to a special variable.

The variable whose value was rejected is:

    SHARED_LIBRARY_NAME
The value being written to it was of the following type:

    str

This variable expects the following type(s):

    unicode

Change the file to write a value of the appropriate type and try again.

>> +            exec code in self
>
> Why did you change this from exec(code, self)?

Because not changing it leads to:
  SyntaxError: unqualified exec is not allowed in function 'execute' it is a nested function
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.