Replace the @advanced decorator

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
from bug 1256568 comment 11:

(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not too keen on all this delayed imports for stdlib modules that will
> almost certainly always be loaded. But if that's the style we're using for
> moz.configure, so be it.

I've been thinking about replacing the @advanced decorator with a @import
decorator. Which would have the double advantage of documenting which modules
are required for the function without looking at its AST, and would solve your
concern.
(Assignee)

Updated

2 years ago
Blocks: 1257663
No longer blocks: 1247781
(Assignee)

Updated

2 years ago
Depends on: 1259351
(Assignee)

Updated

2 years ago
Blocks: 1260066
(Assignee)

Comment 1

2 years ago
Created attachment 8735315 [details]
MozReview Request: Bug 1256573 - Add missing primitives to the ConfigureSandbox documentation. r?nalexander

Not directly related, but since this bug is about to add another primitive...

Review commit: https://reviewboard.mozilla.org/r/42685/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42685/
Attachment #8735315 - Flags: review?(nalexander)
Attachment #8735316 - Flags: review?(nalexander)
Attachment #8735317 - Flags: review?(nalexander)
Attachment #8735318 - Flags: review?(nalexander)
Attachment #8735319 - Flags: review?(nalexander)
(Assignee)

Comment 2

2 years ago
Created attachment 8735316 [details]
MozReview Request: Bug 1256573 - Add a new @imports primitive that allows to import modules into the decorated functions. r?nalexander

Currently, we have @advanced, that gives the decorated functions access
to all the builtins and consequently, to the import statement.
That is a quite broad approach and doesn't allow to easily introspect
what functions are importing which modules.

This change introduces a new decorator that allows to import modules one
by one into the decorated functions.

Review commit: https://reviewboard.mozilla.org/r/42687/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42687/
(Assignee)

Comment 3

2 years ago
Created attachment 8735317 [details]
MozReview Request: Bug 1256573 - Switch configure tests to use @imports instead of @advanced. r?nalexander

Review commit: https://reviewboard.mozilla.org/r/42689/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42689/
(Assignee)

Comment 4

2 years ago
Created attachment 8735318 [details]
MozReview Request: Bug 1256573 - Switch moz.configure to use @imports instead of @advanced. r?nalexander

Review commit: https://reviewboard.mozilla.org/r/42691/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42691/
(Assignee)

Comment 5

2 years ago
Created attachment 8735319 [details]
MozReview Request: Bug 1256573 - Remove the @advanced primitive. r?nalexander

Review commit: https://reviewboard.mozilla.org/r/42693/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42693/
(Assignee)

Updated

2 years ago
Blocks: 1259272
Comment on attachment 8735315 [details]
MozReview Request: Bug 1256573 - Add missing primitives to the ConfigureSandbox documentation. r?nalexander

https://reviewboard.mozilla.org/r/42685/#review39321

::: python/mozbuild/mozbuild/configure/__init__.py:61
(Diff revision 1)
>      """Represents a sandbox for executing Python code for build configuration.
>      This is a different kind of sandboxing than the one used for moz.build
>      processing.
>  
> -    The sandbox has 5 primitives:
> +    The sandbox has 8 primitives:
>      - option

As before, consider putting the type information with the name, like:

- option (function X -> Y)
- foo (decorator)
Attachment #8735315 - Flags: review?(nalexander) → review+
Comment on attachment 8735316 [details]
MozReview Request: Bug 1256573 - Add a new @imports primitive that allows to import modules into the decorated functions. r?nalexander

https://reviewboard.mozilla.org/r/42687/#review39325

lgtm.

::: python/mozbuild/mozbuild/configure/__init__.py:453
(Diff revision 1)
> +                (_import, True), (_from, False), (_as, False)):
> +            if not isinstance(value, types.StringTypes) and not (
> +                    required or value is None):
> +                raise TypeError("Unexpected type: '%s'" % type(value))
> +            if value is not None and not self.RE_MODULE.match(value):
> +                raise ValueError("Invalid argument to @imports: '%s'" % value)

Consider blacklisting `__builtin__` when you're done removin `@advanced`.

::: python/mozbuild/mozbuild/configure/__init__.py:459
(Diff revision 1)
> +
> +        def decorator(func):
> +            if func in self._prepared_functions:
> +                raise ConfigureError(
> +                    '@imports must appear after other decorators')
> +            # For the imports to apply in the order they appear in the

Why is it important that imports are imported in order?  Do you anticipate consumers needing to import transitive dependencies?  (I really hope not.)

::: python/mozbuild/mozbuild/configure/__init__.py:482
(Diff revision 1)
> +            # restricted mode"
> +            if _from == '__builtin__' and _import == 'open':
> +                glob[_as or _import] = \
> +                    lambda *args, **kwargs: open(*args, **kwargs)
> +                continue
> +            # Until this proves to be a performance problem. just construct an

nit: s/./,/

::: python/mozbuild/mozbuild/test/configure/test_configure.py:244
(Diff revision 1)
> +        out = StringIO()
> +        sandbox = ConfigureSandbox(config, {}, [], out, out)
> +
> +        with self.assertRaises(ImportError):
> +            exec(
> +                '@template\n'

I always like triple quoted strings for this.  Or splitting a file on some separator.

::: python/mozbuild/mozbuild/test/configure/test_configure.py:269
(Diff revision 1)
> +            'def foo():\n'
> +            '    return path\n',
> +            sandbox
> +        )
> +
> +        self.assertIs(sandbox['foo'](), os.path)

Worth asserting the sandbox does get other things, like `'os'` itself, in at least one place.  That is, make a function that returns the function globals, and diff against a fresh sandbox to ensure things aren't leaking.
Attachment #8735316 - Flags: review?(nalexander) → review+
Comment on attachment 8735317 [details]
MozReview Request: Bug 1256573 - Switch configure tests to use @imports instead of @advanced. r?nalexander

https://reviewboard.mozilla.org/r/42689/#review39327
Attachment #8735317 - Flags: review?(nalexander) → review+
Comment on attachment 8735318 [details]
MozReview Request: Bug 1256573 - Switch moz.configure to use @imports instead of @advanced. r?nalexander

https://reviewboard.mozilla.org/r/42691/#review39329

Stamp!

::: build/moz.configure/init.configure:301
(Diff revision 1)
>           'ZIP',
>       ])
>  
>  
>  @depends(mozconfig, wanted_mozconfig_variables, '--help')
> +# This gives access to the sandbox. Don't copy this blindly.

Consider "requiring" build peer review instead.  Better to give instructions than prohibit, I think.

::: build/moz.configure/util.configure:44
(Diff revision 1)
> -@advanced
> +# This unlocks the sandbox. Do not copy blindly.
> +@imports(_import='__builtin__', _as='__builtins__')
>  def find_program(file):
>      if is_absolute_or_relative(file):
>          return os.path.abspath(file) if os.path.isfile(file) else None
> +    # We can't use @imports here because it imports at declaration time,

Sad panda.  Do you have thoughts on how this happens?
Attachment #8735318 - Flags: review?(nalexander) → review+
Comment on attachment 8735319 [details]
MozReview Request: Bug 1256573 - Remove the @advanced primitive. r?nalexander

https://reviewboard.mozilla.org/r/42693/#review39333
Attachment #8735319 - Flags: review?(nalexander) → review+
(Assignee)

Comment 11

2 years ago
(In reply to Nick Alexander :nalexander from comment #7)
> Comment on attachment 8735316 [details]
> MozReview Request: Bug 1256573 - Add a new @imports primitive that allows to
> import modules into the decorated functions. r?nalexander
> 
> https://reviewboard.mozilla.org/r/42687/#review39325
> 
> lgtm.
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:453
> (Diff revision 1)
> > +                (_import, True), (_from, False), (_as, False)):
> > +            if not isinstance(value, types.StringTypes) and not (
> > +                    required or value is None):
> > +                raise TypeError("Unexpected type: '%s'" % type(value))
> > +            if value is not None and not self.RE_MODULE.match(value):
> > +                raise ValueError("Invalid argument to @imports: '%s'" % value)
> 
> Consider blacklisting `__builtin__` when you're done removin `@advanced`.

It's still needed for various things, but only one place needs to do "import __builtin__ as __builtins__", now (which is the equivalent of advanced).

> ::: python/mozbuild/mozbuild/configure/__init__.py:459
> (Diff revision 1)
> > +
> > +        def decorator(func):
> > +            if func in self._prepared_functions:
> > +                raise ConfigureError(
> > +                    '@imports must appear after other decorators')
> > +            # For the imports to apply in the order they appear in the
> 
> Why is it important that imports are imported in order?  Do you anticipate
> consumers needing to import transitive dependencies?  (I really hope not.)

It's probably not important. That said, applying imports as the function is called is not going to pan out either because I'd like to delay the imports anyways (that's the only way for the last use of the equivalent of advanced to go away)
(Assignee)

Comment 12

2 years ago
(In reply to Nick Alexander :nalexander from comment #6)
> As before, consider putting the type information with the name, like:
> 
> - option (function X -> Y)
> - foo (decorator)

It made sense in the previous case, but in this one, every one of those primitives has its own documentation...
(Assignee)

Comment 13

2 years ago
(In reply to Nick Alexander :nalexander from comment #7)
> Worth asserting the sandbox does get other things, like `'os'` itself, in at
> least one place.  That is, make a function that returns the function
> globals, and diff against a fresh sandbox to ensure things aren't leaking.

I'll go with a simpler check:
    self.assertEquals(sandbox.keys(), ['__builtins__', 'foo'])
    self.assertEquals(sandbox['__builtins__'], ConfigureSandbox.BUILTINS)
(Assignee)

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/42691/#review39329

> Consider "requiring" build peer review instead.  Better to give instructions than prohibit, I think.

Come to think of it, I'd rather keep it strictly prohibitive, and even enforce it during make check after bug 1259272.

Updated

6 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.