Add a only_when context manager to the python configure sandbox

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 3 bugs)

unspecified
mozilla52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(14 attachments)

58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
Assignee

Description

3 years ago
From https://groups.google.com/d/msg/mozilla.dev.builds/XThvzlFdlKU/vzi2PMKICQAJ :

>  (...) I've been revisiting the idea from bug 1259272 to have a
>  tool to do some introspection, and that not everything is visible can
>  make it awkward to use such a tool. The worst offender being
>  --enable-project/--enable-application.
>
>  I came up with a prototype patch that seems to work and would make
>  everything known. The idea is to add a context manager to the sandbox,
>  that works as follows:
>      with only_when(<some_depends_function>):
>          python_configure_stuff()
>
>  We can then rewrite things like depends_when or include_when using
>  that context manager. For example, include_when (which is the only
>  thing I tested so far) becomes:
>      @template
>      def include_when(filename, when):
>          with only_when(when):
>              include(filename)
>
>  I expect it would also allow to remove depends_win from
>  windows.configure, for example.
Assignee

Updated

3 years ago
Blocks: 1291356
Assignee

Updated

3 years ago
No longer blocks: 1291356
Assignee

Updated

3 years ago
No longer blocks: 1297471
Assignee

Updated

3 years ago
Blocks: 1308774
Assignee

Comment 1

3 years ago
Let's go with what I have currently, which brings the only_when context manager as well as a `when` argument to all functions. This gets us in a place where we can remove include_when, and @depends on things that are defined in files included conditionally. We can't remove @depends_when yet because that would require adding a lot of --help dependencies. I want to get rid of the --help dependencies first, and I'll do that in a separate bug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

3 years ago
mozreview-review
Comment on attachment 8800949 [details]
Bug 1296530 - Add a repr() for Option.

https://reviewboard.mozilla.org/r/85732/#review85182
Attachment #8800949 - Flags: review?(cmanchester) → review+

Comment 17

3 years ago
mozreview-review
Comment on attachment 8800950 [details]
Bug 1296530 - Rename DependsFunction to SandboxDependsFunction.

https://reviewboard.mozilla.org/r/85734/#review85184
Attachment #8800950 - Flags: review?(cmanchester) → review+

Comment 18

3 years ago
mozreview-review
Comment on attachment 8800951 [details]
Bug 1296530 - Store DependsFunction information for the sandbox as class instances instead of tuples.

https://reviewboard.mozilla.org/r/85736/#review85186
Attachment #8800951 - Flags: review?(cmanchester) → review+

Comment 19

3 years ago
mozreview-review
Comment on attachment 8800952 [details]
Bug 1296530 - Move more things in the new DependsFunction and add a repr() for it.

https://reviewboard.mozilla.org/r/85738/#review85188
Attachment #8800952 - Flags: review?(cmanchester) → review+

Comment 20

3 years ago
mozreview-review
Comment on attachment 8800953 [details]
Bug 1296530 - Move @depends dependency resolution to a separate function.

https://reviewboard.mozilla.org/r/85740/#review85190
Attachment #8800953 - Flags: review?(cmanchester) → review+

Comment 21

3 years ago
mozreview-review
Comment on attachment 8800954 [details]
Bug 1296530 - Move the somehow redundant check for --help dependency from _resolve to _value_for_depends.

https://reviewboard.mozilla.org/r/85742/#review85192
Attachment #8800954 - Flags: review?(cmanchester) → review+

Comment 22

3 years ago
mozreview-review
Comment on attachment 8800955 [details]
Bug 1296530 - Get help text out of TestConfigure.get_config when given --help.

https://reviewboard.mozilla.org/r/85744/#review85194
Attachment #8800955 - Flags: review?(cmanchester) → review+

Comment 23

3 years ago
mozreview-review
Comment on attachment 8800956 [details]
Bug 1296530 - Add a test for unexpected keyword argument to @depends.

https://reviewboard.mozilla.org/r/85746/#review85196
Attachment #8800956 - Flags: review?(cmanchester) → review+

Comment 24

3 years ago
mozreview-review
Comment on attachment 8800957 [details]
Bug 1296530 - Add a `when` argument to @depends().

https://reviewboard.mozilla.org/r/85748/#review85198
Attachment #8800957 - Flags: review?(cmanchester) → review+

Comment 25

3 years ago
mozreview-review
Comment on attachment 8800958 [details]
Bug 1296530 - Add a `when` argument to option().

https://reviewboard.mozilla.org/r/85750/#review85208

::: python/mozbuild/mozbuild/configure/__init__.py:512
(Diff revision 1)
> +            if c != when:
> +                raise ConfigureError('@depends function needs the same `when` '
> +                                     'as options it depends on')

It seems like the criteria might be that a `when` condition for a depends function should be at least as restrictive as the conjunction of the `when` conditions of all the options it depends on. I guess we'll see if this is an issue in practice.
Attachment #8800958 - Flags: review?(cmanchester) → review+

Comment 26

3 years ago
mozreview-review
Comment on attachment 8800959 [details]
Bug 1296530 - Add a `when` argument to set_config() and set_define().

https://reviewboard.mozilla.org/r/85752/#review85220
Attachment #8800959 - Flags: review?(cmanchester) → review+

Comment 27

3 years ago
mozreview-review
Comment on attachment 8800960 [details]
Bug 1296530 - Add a `when` argument to imply_option().

https://reviewboard.mozilla.org/r/85754/#review85222
Attachment #8800960 - Flags: review?(cmanchester) → review+

Comment 28

3 years ago
mozreview-review
Comment on attachment 8800961 [details]
Bug 1296530 - Add an only_when context manager, and a `when` argument to include().

https://reviewboard.mozilla.org/r/85756/#review85256

::: python/mozbuild/mozbuild/configure/__init__.py:526
(Diff revision 1)
> +
> +        `only_when` is a context manager that essentially makes calls to
> +        other sandbox functions within the context block ignored.
> +        '''
> +        when = self._normalize_when(when, 'only_when')
> +        if when and self._default_conditions[-1:] != [when]:

Can this be `if when and when not in self._default_conditions:`?
Attachment #8800961 - Flags: review?(cmanchester) → review+

Comment 29

3 years ago
mozreview-review
Comment on attachment 8800962 [details]
Bug 1296530 - Replace include_when with include, and remove include_when.

https://reviewboard.mozilla.org/r/85758/#review85268

Very nice.
Attachment #8800962 - Flags: review?(cmanchester) → review+
Assignee

Comment 30

3 years ago
(In reply to Chris Manchester (:chmanchester) from comment #25)
> Comment on attachment 8800958 [details]
> Bug 1296530 - Add a `when` argument to option().
> 
> https://reviewboard.mozilla.org/r/85750/#review85208
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:512
> (Diff revision 1)
> > +            if c != when:
> > +                raise ConfigureError('@depends function needs the same `when` '
> > +                                     'as options it depends on')
> 
> It seems like the criteria might be that a `when` condition for a depends
> function should be at least as restrictive as the conjunction of the `when`
> conditions of all the options it depends on. I guess we'll see if this is an
> issue in practice.

Yeah, I went with "simple" for now.

(In reply to Chris Manchester (:chmanchester) from comment #28)
> Comment on attachment 8800961 [details]
> Bug 1296530 - Add an only_when context manager, and a `when` argument to
> include().
> 
> https://reviewboard.mozilla.org/r/85756/#review85256
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:526
> (Diff revision 1)
> > +
> > +        `only_when` is a context manager that essentially makes calls to
> > +        other sandbox functions within the context block ignored.
> > +        '''
> > +        when = self._normalize_when(when, 'only_when')
> > +        if when and self._default_conditions[-1:] != [when]:
> 
> Can this be `if when and when not in self._default_conditions:`?

Mmmmm maybe it could, but I'd rather keep it simple for now. That is, only deal with the case where the new when is the same as the last one that was pushed.

Comment 31

3 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/c389fc412bb4
Add a repr() for Option. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/108c876dc9fe
Rename DependsFunction to SandboxDependsFunction. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/255afed66a57
Store DependsFunction information for the sandbox as class instances instead of tuples. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/2c4e06f0fb98
Move more things in the new DependsFunction and add a repr() for it. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/882ca1a77bcd
Move @depends dependency resolution to a separate function. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/3cd77670c896
Move the somehow redundant check for --help dependency from _resolve to _value_for_depends. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/9ae12fd1dd3d
Get help text out of TestConfigure.get_config when given --help. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/45b4e523d944
Add a test for unexpected keyword argument to @depends. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8641bc42dccc
Add a `when` argument to @depends(). r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/38130e1d78cb
Add a `when` argument to option(). r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/c728d106f0d8
Add a `when` argument to set_config() and set_define(). r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/7b7af91ddac6
Add a `when` argument to imply_option(). r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8bc8d974ae10
Add an only_when context manager, and a `when` argument to include(). r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/793b6eeb323a
Replace include_when with include, and remove include_when. r=chmanchester

Updated

3 years ago
Depends on: 1311045

Updated

3 years ago
Depends on: 1311069
Assignee

Updated

3 years ago
Blocks: 1316957
Assignee

Updated

2 years ago
Blocks: 1335666

Updated

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