Closed Bug 1134072 Opened 8 years ago Closed 7 years ago

Support for sub-contexts in moz.build sandboxes

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 1 obsolete file)

1.38 KB, patch
gps
: review+
Details | Diff | Splinter Review
1.41 KB, patch
gps
: review+
Details | Diff | Splinter Review
3.76 KB, patch
gps
: review+
Details | Diff | Splinter Review
4.07 KB, patch
Details | Diff | Splinter Review
11.65 KB, patch
Details | Diff | Splinter Review
14.99 KB, patch
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
39 bytes, text/x-review-board-request
glandium
: review+
Details
We want something like the following in moz.build files:

with X():
    BAR = True
    BAZ = False

The activation of the context manager will result in the context associated with the sandbox to be swapped for a new one of a specific type. When that context manager is active, assignments to UPPERCASE variables will be directed towards the new context.

When the context manager exits, the context associated with it is emitted.

From the perspective of the reader, we'll see something like:

  [SubContext1, SubContext2, Context]

This approach could eventually allow us to group variables into specific blocks. e.g.

with Testing():
    XPCSHELL_MANIFESTS += ['foo.ini']

This could also lead to the global symbols namespace being less polluted. This would increase domain coherence and increase comprehension.

But all that is for the future. This feature is being designed with supporting bug 1132771 in mind.
FWIW, the typical cases I had in mind when I thought about this syntax are:

with Library('foo'):
    SOURCES += [...]

with GYP_DIRS['foo']:
    INPUT = 'foo/foo.gyp'
    VARIABLES = { ... }
Attached file MozReview Request: bz://1134072/gps (obsolete) —
/r/4033 - Bug 1134072 - Ability to turn Context instances into context managers
/r/4035 - Bug 1134072 - Support variables derived from sandboxes

Pull down these commits:

hg pull review -r 5ede7f776deee10af6e6ebe550ececc6b6e6e9fc
Attachment #8566346 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/4031/#review3179

It turned out to be difficult to separate this work from bug 1132771. I'll post a full series in the other review request so you can see how this plays out.
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4081 - Bug 1134072 - Remove support for post-eval sandbox callback
/r/4033 - Bug 1134072 - Ability to turn Context instances into context managers
/r/4035 - Bug 1134072 - Support variables derived from sandboxes
/r/4083 - Bug 1134072 - Record sub-contexts on sandbox instances
/r/4085 - Bug 1134072 - Emit sub-contexts during reading

Pull down these commits:

hg pull review -r 81b7712f83da3dda98c8e22f215a9bf662fdf05d
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4081 - Bug 1134072 - Remove support for post-eval sandbox callback
/r/4083 - Bug 1134072 - Support for sub-contexts

Pull down these commits:

hg pull review -r 3c034dd248df1979f30fe8559eb296edb19e3c56
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4081 - Bug 1134072 - Remove support for post-eval sandbox callback
/r/4083 - Bug 1134072 - Support for sub-contexts
/r/4127 - Bug 1134072 - Declare sub-contexts via SUBCONTEXTS variable

Pull down these commits:

hg pull review -r 6bcb97463173fc22fd0c0b96d4773555abed9b42
After thinking about this, and the future, here is a counter proposal, in 6 parts, the first 2 of which are generic fixes the 4 others depend on.
Attachment #8567832 - Flags: review?(gps)
This part needs some bikeshedding, as to how to declare the variable set.
Attachment #8567841 - Flags: feedback?(gps)
Attachment #8567832 - Flags: review?(gps) → review+
Attachment #8567834 - Flags: review?(gps) → review+
Comment on attachment 8567835 [details] [diff] [review]
Part 3 - Keep the template name in TemplateContext instances

Review of attachment 8567835 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is generic enough to accept without considering the patches afterwards.
Attachment #8567835 - Flags: review?(gps) → review+
Comment on attachment 8567839 [details] [diff] [review]
Part 5 - Allow invoking moz.build templates as context managers

Review of attachment 8567839 [details] [diff] [review]:
-----------------------------------------------------------------

Just granting f+ for now.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +381,5 @@
> +            self._sandbox_context = self._sandbox._context
> +            self._sandbox._context = self._context
> +
> +        def __exit__(self, type, value, traceback):
> +            self._sandbox._extra_contexts.append(self._context)

I may have made this same choice, but putting the append in __exit__ means we have LIFO not FIFO for stacked context managers. I think emitting the context managers in the order they were defined in source makes more sense. This could be a problem if we ever consume context managers in the middle of file execution. But I don't see us treating sandbox evaluation as something non-atomic.

@@ +1002,1 @@
>              self._sandbox_post_eval_cb(context)

Feel free to cherry-pick my patch that kills sandbox_post_eval_cb.

@@ +1002,4 @@
>              self._sandbox_post_eval_cb(context)
>  
> +        for ctxt in ctxts:
> +            yield ctxt

This emits the sub-contexts before the parent. I'm not sure if this is good or bad. Perhaps it doesn't matter.
Attachment #8567839 - Flags: review?(gps) → feedback+
What I like about your series:

* Sub-contexts and templates are the same thing
* Sub-contexts are defined in "userland", not as part of the mozbuild Python package

What I like about my series:

* Assignments within sub-contexts don't leak into the outer context
* I don't have to rebase my work to land it :)

I think we agree that sub-contexts and templates are pretty much the same thing. I realized this late last week. When I went to bed last night, I thought I might be spending the day merging them. But since you beat me to it...

A concern I have with pure userland sub-contexts is how we're going to facilitate some advanced functionality. For example, Files has some custom types and has a __iadd__ implementation relying on a bunch of symbols not exported to moz.build files (whether this should be __iadd__ is debatable - but if we are declaring Files metadata in userland (via templates), then the code for merging that metadata must also be declared in userland so to avoid version conflicts. Furthermore, if memory serves, we both have this idea of moving more advanced logic into moz.build execution (from emitter.py and even .mk files). This will require us to do more and more advanced Python in moz.build execution. Perhaps we give files defining templates/sub-contexts a more complete sandbox from which to construct advanced functionality?

Anyway, we should probably hash this out on IRC so we don't continue to see past each other. I've already pinged you.
(In reply to Gregory Szorc [:gps] from comment #15)
> * Assignments within sub-contexts don't leak into the outer context

Not sure what you mean here, but

with Template():
    FOO = ...

does not make FOO available outside the subcontext. However, local variables *are* shared, and that's actually an intentional feature, although I don't envision this to be more useful than being able to do something like:

foo = ...

with Template('bar'):
    FOO = foo

with Template('baz'):
    FOO = foo
(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8567839 [details] [diff] [review]
> Part 5 - Allow invoking moz.build templates as context managers
> 
> Review of attachment 8567839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just granting f+ for now.
> 
> ::: python/mozbuild/mozbuild/frontend/reader.py
> @@ +381,5 @@
> > +            self._sandbox_context = self._sandbox._context
> > +            self._sandbox._context = self._context
> > +
> > +        def __exit__(self, type, value, traceback):
> > +            self._sandbox._extra_contexts.append(self._context)
> 
> I may have made this same choice, but putting the append in __exit__ means
> we have LIFO not FIFO for stacked context managers. I think emitting the
> context managers in the order they were defined in source makes more sense.

So you're saying that

with A():
    with B():
        pass

should emit A before B? I'm not sure this is actually desirable. OTOH, I'm not even sure it matters at all.
Comment on attachment 8567837 [details] [diff] [review]
Part 4 - Move moz.build template handling in a separate class

Review of attachment 8567837 [details] [diff] [review]:
-----------------------------------------------------------------

Remove the __del__ to make this an r+.

A clever way of doing that might be to implement __call__ on this class, instead of doing that from __init__.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +371,5 @@
>                  sandbox[k] = v
>  
>              sandbox.exec_source(code, path)
>  
> +        def __del__(self):

__del__ is kinda evil. Can you make this a regular function and call it explicitly?

@@ +376,3 @@
>              # This is gross, but allows the merge to happen. Eventually, the
>              # merging will go away and template contexts emitted independently.
> +            klass = self._sandbox._context.__class__

"old_class" makes the intention of this variable clearer.
Attachment #8567837 - Flags: review?(gps) → feedback+
Keywords: leave-open
https://reviewboard.mozilla.org/r/4083/#review3355

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +        self._all_paths = list(parent._all_paths)

I think this would be better as:
  for p in parent.source_stack:
      self.push_source(p)

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +            sandbox.subcontexts.append(self)

I would feel better if the synchronization between _active_contexts and subcontext was done on the sandbox end.

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> +        Context.__init__(self, **kwargs)

Context.\_\_init\_\_ only has two parameters, one of which you're overriding with the parent data.

I think you should make that:
  def \_\_init\_\_(self, parent, allowed_variables=\{\}):
      Context.\_\_init(self, allowed_variables=allowed_variables, config=parent.config)

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 3)
> +        context._sandbox = weakref.ref(self)

I think it would be better for the sandbox to be set (and unset) around the exec in Sandbox.exec_source. And if the context already has a sandbox attached, that would be saved and restored.
https://reviewboard.mozilla.org/r/4127/#review3357

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 1)
> +SUBCONTEXTS = [

I'd argue everything below this point (except the reference to Sphinx) should be folded in the previous patch.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 1)
> -        if key in SPECIAL_VARIABLES or key in FUNCTIONS:
> +        if key in SPECIAL_VARIABLES or key in FUNCTIONS or key in SUBCONTEXTS:

Thought: we should probably add key in self.templates here.

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 1)
> +            return cls(self._context, allowed_variables=cls.VARIABLES,

Since you're making cls.VARIABLES mandatory, it strikes me that SubContext should just not take an allowed_variables argument, and the Context.__init__ call in SubContext.__init__ could just be Context.__init__(self, self.VARIABLES, parent.config)
I'll file a separate bug for part 1 & 2 ; I'll leave part 3 out for now, because I can see ways to rewrite part 4 to 6 on top of your queue that don't involve part 3.
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4127 - Bug 1134072 - Support for sub-contexts

Pull down this commit:

hg pull review -r d6ce8a5ab5bb73496faf441c91b25b449fa7945f
https://reviewboard.mozilla.org/r/4127/#review3375

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 2)
> +        self.current_path = parent.current_path

push_source will fill both main_path and current_path

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 2)
> +        self.config = parent.config

You don't need this anymore, since you're passing config to Context.__init__.

::: python/mozbuild/mozbuild/frontend/sandbox.py
(Diff revision 2)
> +        popped = self._active_contexta.pop()

typo
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4127 - Bug 1134072 - Support for sub-contexts

Pull down this commit:

hg pull review -r affaf50186644d4c8b6ee4c5d617b1d34f75cf08
https://reviewboard.mozilla.org/r/4127/#review3417

> Thought: we should probably add key in self.templates here.

I did this and it breaks moz.build processing. I didn't dig deeper.
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

/r/4127 - Bug 1134072 - Support for sub-contexts
/r/4253 - Bug 1134072 - Create a dedicated type for the main moz.build Context

Pull down these commits:

hg pull review -r 6e035cfd1e05697c6be3e679d95fbd5a4bb6649e
https://reviewboard.mozilla.org/r/4253/#review3439

::: python/mozbuild/mozbuild/frontend/reader.py
(Diff revision 1)
> +    belonging to the main moz.build file from other Context instances.

This doesn't seem necessary to me. Either you have a Context, and it's a main context, or you have a subclass of SubContext, and it's a subcontext.
https://reviewboard.mozilla.org/r/4253/#review3447

> This doesn't seem necessary to me. Either you have a Context, and it's a main context, or you have a subclass of SubContext, and it's a subcontext.

I'll drop this commit.
Attachment #8567841 - Flags: feedback?(gps)
Comment on attachment 8566346 [details]
MozReview Request: bz://1134072/gps

r+ with the second part dropped.
Attachment #8566346 - Flags: review?(mh+mozilla) → review+
Keywords: leave-open
Blocks: 1136552
https://hg.mozilla.org/mozilla-central/rev/13371429d31f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8566346 - Attachment is obsolete: true
Attachment #8619501 - Flags: review+
Attachment #8619502 - Flags: review+
Attachment #8619503 - Flags: review+
Attachment #8619504 - Flags: review+
Attachment #8619505 - Flags: review+
Attachment #8619506 - Flags: review+
Attachment #8619507 - Flags: review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.