Move our windows multiprocessing hacks to a central location so we can use them for config.status as well

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

unspecified
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

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.
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.
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.
(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

3 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+
Assignee

Updated

3 years ago
Depends on: 1317778
Comment hidden (mozreview-request)

Comment 7

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f85e7bfe97d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.