Mike Hommey [:glandium] Assignee
	Description • 8 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.

	Mike Hommey [:glandium] Assignee
	Comment 1 • 8 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.

	Chris Manchester (limited bugmail, email directly)
	Comment 16 • 8 years ago
mozreview-review

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

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

	Chris Manchester (limited bugmail, email directly)
	Comment 17 • 8 years ago
mozreview-review

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

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

	Chris Manchester (limited bugmail, email directly)
	Comment 18 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 19 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 20 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 21 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 22 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 23 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 24 • 8 years ago
mozreview-review

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

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

	Chris Manchester (limited bugmail, email directly)
	Comment 25 • 8 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.

	Chris Manchester (limited bugmail, email directly)
	Comment 26 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 27 • 8 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

	Chris Manchester (limited bugmail, email directly)
	Comment 28 • 8 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:`?

	Chris Manchester (limited bugmail, email directly)
	Comment 29 • 8 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.

	Mike Hommey [:glandium] Assignee
	Comment 30 • 8 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.

	Pulsebot
	Comment 31 • 8 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

	Pulsebot
	Comment 32 • 8 years ago

Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79c148f36458
Bustage followup; r=gps

	Pulsebot
	Comment 33 • 8 years ago

Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44d29bac3654
Bustage followup part deux; r=gps

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago
bugherder

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