Closed
Bug 1256571
Opened 8 years ago
Closed 8 years ago
Change the execution model for python configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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).
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
Note the current execution model, in that case, makes the options created by check_prog within a @depends function invisible to the help...
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Blocks: pyconfigure-introspect
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
instead of manually reading the member variable containing the results. Review commit: https://reviewboard.mozilla.org/r/45949/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45949/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45951/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45951/
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45953/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45953/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45955/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45955/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45957/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45957/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45959/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45959/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45961/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45961/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45963/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45963/
Assignee | ||
Comment 12•8 years ago
|
||
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/
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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 15•8 years ago
|
||
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 16•8 years ago
|
||
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 17•8 years ago
|
||
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 18•8 years ago
|
||
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 19•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8740720 -
Flags: review?(cmanchester) → review+
Comment 20•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8740721 -
Flags: review?(cmanchester) → review+
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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+
Assignee | ||
Comment 23•8 years ago
|
||
(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.
Assignee | ||
Comment 24•8 years ago
|
||
(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.
Assignee | ||
Comment 25•8 years ago
|
||
(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.
Assignee | ||
Comment 26•8 years ago
|
||
(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.
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2f69811b52 https://hg.mozilla.org/integration/mozilla-inbound/rev/22382dc2c9fa https://hg.mozilla.org/integration/mozilla-inbound/rev/f6dba01913b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd34b3b1cf9 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0fa338d4bf https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9713988f7a https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6bd5ce0abb https://hg.mozilla.org/integration/mozilla-inbound/rev/2916126df668 https://hg.mozilla.org/integration/mozilla-inbound/rev/02561278899c https://hg.mozilla.org/integration/mozilla-inbound/rev/25f299dca206
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e2f69811b52 https://hg.mozilla.org/mozilla-central/rev/22382dc2c9fa https://hg.mozilla.org/mozilla-central/rev/f6dba01913b9 https://hg.mozilla.org/mozilla-central/rev/dbd34b3b1cf9 https://hg.mozilla.org/mozilla-central/rev/7e0fa338d4bf https://hg.mozilla.org/mozilla-central/rev/cc9713988f7a https://hg.mozilla.org/mozilla-central/rev/ae6bd5ce0abb https://hg.mozilla.org/mozilla-central/rev/2916126df668 https://hg.mozilla.org/mozilla-central/rev/02561278899c https://hg.mozilla.org/mozilla-central/rev/25f299dca206
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•