Closed
Bug 1316140
Opened 8 years ago
Closed 8 years ago
Move our windows multiprocessing hacks to a central location so we can use them for config.status as well
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(1 file)
I found we hit bug 914563 as seen as we try to use multiprocessing as a part of build-backend, because we can end up with "config.status" as the file for our main module. This bug is about moving the monkey patching we currently do for "mach" to a central location so we can use it for "config.status" as well.
Comment 1•8 years ago
|
||
I'm not a fan of the multiprocessing module because it is brittle [on Windows]. If the opportunity is there, converting to concurrent.futures.ProcessPoolExecutor is one way to solve the problem.
Assignee | ||
Comment 2•8 years ago
|
||
I should have said I'm already using ProcessPoolExecutor. I mention multiprocessing because concurrent.futures is implemented with multiprocessing, and that's where the error is.
Comment 3•8 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #2)
> I should have said I'm already using ProcessPoolExecutor. I mention
> multiprocessing because concurrent.futures is implemented with
> multiprocessing, and that's where the error is.
TIL. FML.
Unless you absolutely need the "pool" bits, ThreadPoolExecutor + subprocess.call() might even be preferable. I've just had so many bad experiences with multiprocessing over the years that I try to avoid it.
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8809095 [details]
Bug 1316140 - Allow use of multiprocessing from config.status on windows.
https://reviewboard.mozilla.org/r/91746/#review92366
Looks good! I think patch_main() should probably have a different home before landing, though.
::: configure.py:82
(Diff revision 1)
> "'non_global_defines', 'substs', 'mozconfig']")
>
> if config.get('MOZ_BUILD_APP') != 'js' or config.get('JS_STANDALONE'):
> fh.write(textwrap.dedent('''
> if __name__ == '__main__':
> + from mozbuild.action.patch_main import patch_main
I think mozbuild.action is probably the wrong place for this. That's for python utilities called during build time, like buildlist or the preprocessor:
https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/config/rules.mk#1425
https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/config/makefiles/xpidl/Makefile.in#56
I don't think we'd want to $(call py_action,patch_main) anywhere, right? Maybe mozbuild/util.py is a better fit.
Attachment #8809095 -
Flags: review?(mshal) → review+
Comment hidden (mozreview-request) |
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f85e7bfe97d
Allow use of multiprocessing from config.status on windows. r=mshal
Comment 8•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•