Closed Bug 1256571 Opened 5 years ago Closed 5 years ago

Change the execution model for python configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 3 open bugs)

Details

Attachments

(10 files)

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
Currently, things in python configure are executed as they are seen. This has advantages for the simplicity of the sandbox, but it has awkward results, like complaining about unknown options after essentially having run configure entirely. And there is also the horror show I'm adding in bug 1256568.

I want to explore a different execution model, but I don't want to land anything before we have better test coverage for failure modes (bug 1254374).

The base idea is that options and templates would still be executed as they are seen, but depends would be processed afterwards, except the ones that depend on --help, because they have an influence on options. That however makes things awkward for imply_options()...

One thought would be to make imply_option and set_config dummy functions that can be passed to @depends, and that would make the depends function called with the current callbacks, instead of setting them in the @depends function global scope. That would also allow the sandbox to know what functions are calling imply_option or set_config, which is not possible presently (except if we start looking at the AST, but that's way more complex).
I just thought about this after writing the patch for bug 1256587, but the proposed execution model would break using the check_prog template from within a @depends function... need to think more about it.
Note the current execution model, in that case, makes the options created by check_prog within a @depends function invisible to the help...
Blocks: pyconfigure-infra
No longer blocks: pyconfigure
Blocks: 1257541
Review commit: https://reviewboard.mozilla.org/r/45947/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45947/
Attachment #8740713 - Flags: review?(cmanchester)
Attachment #8740714 - Flags: review?(cmanchester)
Attachment #8740715 - Flags: review?(cmanchester)
Attachment #8740716 - Flags: review?(cmanchester)
Attachment #8740717 - Flags: review?(cmanchester)
Attachment #8740718 - Flags: review?(cmanchester)
Attachment #8740719 - Flags: review?(cmanchester)
Attachment #8740720 - Flags: review?(cmanchester)
Attachment #8740721 - Flags: review?(cmanchester)
Attachment #8740722 - Flags: review?(cmanchester)
So far, everything was essentially executed at "declaration". This
made the sandbox code simpler, but to improve on the tooling around
python configure (for tests and introspection), we need to have more
flexibility, which executing everything at declaration doesn't give.

With this change, only @depends functions depending on --help, as
well as templates, are executed at the moment the moz.configure
files are included in the sandbox. The remainder is executed at the
end.

Review commit: https://reviewboard.mozilla.org/r/45965/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45965/
Blocks: 1260327
Comment on attachment 8740713 [details]
MozReview Request: Bug 1256571 - Move resolving @depends dependencies to just before running the decorated function. r?chmanchester

https://reviewboard.mozilla.org/r/45947/#review42689
Attachment #8740713 - Flags: review?(cmanchester) → review+
Comment on attachment 8740714 [details]
MozReview Request: Bug 1256571 - Add a generic method to get the results of Options and DependsFunctions. r?chmanchester

https://reviewboard.mozilla.org/r/45949/#review42693
Attachment #8740714 - Flags: review?(cmanchester) → review+
Comment on attachment 8740715 [details]
MozReview Request: Bug 1256571 - Delay resolving the reason for an implied option. r?chmanchester

https://reviewboard.mozilla.org/r/45951/#review42695
Attachment #8740715 - Flags: review?(cmanchester) → review+
Comment on attachment 8740716 [details]
MozReview Request: Bug 1256571 - Ensure consistent values for variables in closures. r?chmanchester

https://reviewboard.mozilla.org/r/45953/#review42699

Just a question and a nit.

::: python/mozbuild/mozbuild/configure/__init__.py:643
(Diff revision 1)
> +        if func.func_closure:
> +            def makecell(content):
> +                def f():
> +                    content
> +                return f.func_closure[0]
> +
> +            closure = tuple(makecell(cell.cell_contents)
> +                            for cell in func.func_closure)
> +

I see what this is doing, and it seems like a reasonable thing to do, but it's tricky enough that I wonder where, exactly, this is necessary in practice.

::: python/mozbuild/mozbuild/configure/__init__.py:657
(Diff revision 1)
>          func = wraps(func)(types.FunctionType(
>              func.func_code,
>              glob,
>              func.__name__,
>              func.func_defaults,
> -            func.func_closure
> +            func.func_closure and closure

Nit: This is a bit unclear. Can't we just initialize `closure` to `None` and pass it here unconditionally?
Attachment #8740716 - Flags: review?(cmanchester) → review+
Comment on attachment 8740717 [details]
MozReview Request: Bug 1256571 - Move Options handling to ConfigureSandbox._value_for(). r?chmanchester

https://reviewboard.mozilla.org/r/45955/#review42707

::: python/mozbuild/mozbuild/configure/__init__.py:321
(Diff revision 1)
> -        self._option_values[option] = value
> -        self._raw_options[option] = (option_string.split('=', 1)[0]
> +        self._value_for(option)
> +

I don't quite understand this call, because the return value isn't used. Is this just to get errors earlier?
Attachment #8740717 - Flags: review?(cmanchester) → review+
Comment on attachment 8740718 [details]
MozReview Request: Bug 1256571 - Move running @depends functions to ConfigureSandbox._value_for(). r?chmanchester

https://reviewboard.mozilla.org/r/45957/#review42709
Attachment #8740718 - Flags: review?(cmanchester) → review+
Comment on attachment 8740719 [details]
MozReview Request: Bug 1256571 - Move applying implied options to ConfigureSandbox._value_for(). r?chmanchester

https://reviewboard.mozilla.org/r/45959/#review42713

::: python/mozbuild/mozbuild/configure/__init__.py:127
(Diff revision 1)
>          self._raw_options = {}
>  
>          # Store options added with `imply_option`, and the reason they were
>          # added (which can either have been given to `imply_option`, or
>          # inferred.
> -        self._implied_options = {}
> +        self._implied_options = []

A `set` seems like a better fit here.
Attachment #8740719 - Flags: review?(cmanchester) → review+
Attachment #8740720 - Flags: review?(cmanchester) → review+
Comment on attachment 8740720 [details]
MozReview Request: Bug 1256571 - Rename ConfigureSandbox.exec_file to include_file. r?chmanchester

https://reviewboard.mozilla.org/r/45961/#review42715
Attachment #8740721 - Flags: review?(cmanchester) → review+
Comment on attachment 8740721 [details]
MozReview Request: Bug 1256571 - Allow ConfigureSandbox.run to not include a given file. r?chmanchester

https://reviewboard.mozilla.org/r/45963/#review42719
Comment on attachment 8740722 [details]
MozReview Request: Bug 1256571 - Change the execution model of python configure. r?chmanchester

https://reviewboard.mozilla.org/r/45965/#review42725
Attachment #8740722 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester (:chmanchester) from comment #16)
> Comment on attachment 8740716 [details]
> MozReview Request: Bug 1256571 - Ensure consistent values for variables in
> closures. r?chmanchester
> 
> https://reviewboard.mozilla.org/r/45953/#review42699
> 
> Just a question and a nit.
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:643
> (Diff revision 1)
> > +        if func.func_closure:
> > +            def makecell(content):
> > +                def f():
> > +                    content
> > +                return f.func_closure[0]
> > +
> > +            closure = tuple(makecell(cell.cell_contents)
> > +                            for cell in func.func_closure)
> > +
> 
> I see what this is doing, and it seems like a reasonable thing to do, but
> it's tricky enough that I wonder where, exactly, this is necessary in
> practice.

So the thing is, while the new execution model kind of enforces some order, some things can run in an unexpected order.

And if you have something like

@template
def foo():
    value = 1
    @depends(...)
    def bar(...):
        # do something with value
    value = 2

Then you have no guarantee which value is going to be used when bar is executed. With the *current* execution model, bar being a @depends function, it's executed at declaration, which means it uses the first value. With the new model, bar can be executed either at declaration if if depends on --help, which means it uses the first value, or later, using the second...

> ::: python/mozbuild/mozbuild/configure/__init__.py:657
> (Diff revision 1)
> >          func = wraps(func)(types.FunctionType(
> >              func.func_code,
> >              glob,
> >              func.__name__,
> >              func.func_defaults,
> > -            func.func_closure
> > +            func.func_closure and closure
> 
> Nit: This is a bit unclear. Can't we just initialize `closure` to `None` and
> pass it here unconditionally?

Sure.
(In reply to Chris Manchester (:chmanchester) from comment #17)
> Comment on attachment 8740717 [details]
> MozReview Request: Bug 1256571 - Move Options handling to
> ConfigureSandbox._value_for(). r?chmanchester
> 
> https://reviewboard.mozilla.org/r/45955/#review42707
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:321
> (Diff revision 1)
> > -        self._option_values[option] = value
> > -        self._raw_options[option] = (option_string.split('=', 1)[0]
> > +        self._value_for(option)
> > +
> 
> I don't quite understand this call, because the return value isn't used. Is
> this just to get errors earlier?

It enforces keeping the option being handled, keeping the current execution model.
(In reply to Mike Hommey [:glandium] from comment #24)
> It enforces keeping the option being handled, keeping the current execution
> model.

It is, obviously, removed in a subsequent patch.
(In reply to Chris Manchester (:chmanchester) from comment #19)
> Comment on attachment 8740719 [details]
> MozReview Request: Bug 1256571 - Move applying implied options to
> ConfigureSandbox._value_for(). r?chmanchester
> 
> https://reviewboard.mozilla.org/r/45959/#review42713
> 
> ::: python/mozbuild/mozbuild/configure/__init__.py:127
> (Diff revision 1)
> >          self._raw_options = {}
> >  
> >          # Store options added with `imply_option`, and the reason they were
> >          # added (which can either have been given to `imply_option`, or
> >          # inferred.
> > -        self._implied_options = {}
> > +        self._implied_options = []
> 
> A `set` seems like a better fit here.

A set is not ordered, and there is no ordered set type. There's OrderedDict to abuse, but that seems like a stretch. In practice, what we're doing to this list shouldn't impact performance. We can rethink it when it's demonstrated it does.
See Also: → 1385539
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.