1296530 - Add a only_when context manager to the python configure sandbox

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

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 2

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a repr() for Option.

Review commit: https://reviewboard.mozilla.org/r/85732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85732/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Rename DependsFunction to SandboxDependsFunction.

Review commit: https://reviewboard.mozilla.org/r/85734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85734/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Store DependsFunction information for the sandbox as class instances instead of tuples.

Review commit: https://reviewboard.mozilla.org/r/85736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85736/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Move more things in the new DependsFunction and add a repr() for it.

Review commit: https://reviewboard.mozilla.org/r/85738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85738/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Move @depends dependency resolution to a separate function.

Review commit: https://reviewboard.mozilla.org/r/85740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85740/

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Move the somehow redundant check for --help dependency from _resolve to _value_for_depends.

Review commit: https://reviewboard.mozilla.org/r/85742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85742/

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Get help text out of TestConfigure.get_config when given --help.

Review commit: https://reviewboard.mozilla.org/r/85744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85744/

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a test for unexpected keyword argument to @depends.

Review commit: https://reviewboard.mozilla.org/r/85746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85746/

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a `when` argument to @depends().

Review commit: https://reviewboard.mozilla.org/r/85748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85748/

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a `when` argument to option().

Review commit: https://reviewboard.mozilla.org/r/85750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85750/

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a `when` argument to set_config() and set_define().

Review commit: https://reviewboard.mozilla.org/r/85752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85752/

Comment 13

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add a `when` argument to imply_option().

Review commit: https://reviewboard.mozilla.org/r/85754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85754/

Comment 14

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Add an only_when context manager, and a `when` argument to include().

Review commit: https://reviewboard.mozilla.org/r/85756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85756/

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1296530 - Replace include_when with include, and remove include_when.

Review commit: https://reviewboard.mozilla.org/r/85758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85758/

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8800949 [details]
Bug 1296530 - Add a repr() for Option.

https://reviewboard.mozilla.org/r/85732/#review85182

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8800950 [details]
Bug 1296530 - Rename DependsFunction to SandboxDependsFunction.

https://reviewboard.mozilla.org/r/85734/#review85184

Comment 18

[Chris Manchester (limited bugmail, email directly)]

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

Comment 19

[Chris Manchester (limited bugmail, email directly)]

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

Comment 20

[Chris Manchester (limited bugmail, email directly)]

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

https://reviewboard.mozilla.org/r/85740/#review85190

Comment 21

[Chris Manchester (limited bugmail, email directly)]

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

Comment 22

[Chris Manchester (limited bugmail, email directly)]

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

Comment 23

[Chris Manchester (limited bugmail, email directly)]

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

https://reviewboard.mozilla.org/r/85746/#review85196

Comment 24

[Chris Manchester (limited bugmail, email directly)]

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

https://reviewboard.mozilla.org/r/85748/#review85198

Comment 25

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 26

[Chris Manchester (limited bugmail, email directly)]

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

https://reviewboard.mozilla.org/r/85752/#review85220

Comment 27

[Chris Manchester (limited bugmail, email directly)]

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

https://reviewboard.mozilla.org/r/85754/#review85222

Comment 28

[Chris Manchester (limited bugmail, email directly)]

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:`?

Comment 29

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 30

[Mike Hommey [:glandium]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79c148f36458
Bustage followup; r=gps

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44d29bac3654
Bustage followup part deux; r=gps

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c389fc412bb4
https://hg.mozilla.org/mozilla-central/rev/108c876dc9fe
https://hg.mozilla.org/mozilla-central/rev/255afed66a57
https://hg.mozilla.org/mozilla-central/rev/2c4e06f0fb98
https://hg.mozilla.org/mozilla-central/rev/882ca1a77bcd
https://hg.mozilla.org/mozilla-central/rev/3cd77670c896
https://hg.mozilla.org/mozilla-central/rev/9ae12fd1dd3d
https://hg.mozilla.org/mozilla-central/rev/45b4e523d944
https://hg.mozilla.org/mozilla-central/rev/8641bc42dccc
https://hg.mozilla.org/mozilla-central/rev/38130e1d78cb
https://hg.mozilla.org/mozilla-central/rev/c728d106f0d8
https://hg.mozilla.org/mozilla-central/rev/7b7af91ddac6
https://hg.mozilla.org/mozilla-central/rev/8bc8d974ae10
https://hg.mozilla.org/mozilla-central/rev/793b6eeb323a
https://hg.mozilla.org/mozilla-central/rev/79c148f36458
https://hg.mozilla.org/mozilla-central/rev/44d29bac3654