Closed
Bug 1256573
Opened 8 years ago
Closed 8 years ago
Replace the @advanced decorator
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
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•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42689/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42689/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42691/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42691/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42693/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42693/
Assignee | ||
Updated•8 years ago
|
Blocks: pyconfigure-introspect
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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 9•8 years ago
|
||
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 10•8 years ago
|
||
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•8 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•8 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•8 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•8 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.
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f01a17884723 https://hg.mozilla.org/integration/mozilla-inbound/rev/96ef4c62378d https://hg.mozilla.org/integration/mozilla-inbound/rev/89b86fc355d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e24db79431 https://hg.mozilla.org/integration/mozilla-inbound/rev/09c22809d1dc
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f01a17884723 https://hg.mozilla.org/mozilla-central/rev/96ef4c62378d https://hg.mozilla.org/mozilla-central/rev/89b86fc355d7 https://hg.mozilla.org/mozilla-central/rev/f4e24db79431 https://hg.mozilla.org/mozilla-central/rev/09c22809d1dc https://hg.mozilla.org/mozilla-central/rev/8710643c72db
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•