Closed Bug 705479 Opened 13 years ago Closed 13 years ago

Parallel builds (-jN) on Windows should fail early if gmake is being used

Categories

(Firefox Build System :: General, enhancement)

All
Windows 7
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla11

People

(Reporter: jon, Assigned: jon)

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 file, 2 obsolete files)

I ran into the documented problem where doing a parallel build on Windows with gmake will randomly deadlock during the compile process. I got directed to use Pymake instead, which works great for me, but it'd be helpful for new developers if the compile process would stop with an error message if gmake is being used to do a parallel build.
Are Thunderbird or Seamonkey still using VMs for build infra?  IIRC we did parallel make on gmake in VMs without deadlocking because the VMs are so damn slow.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> Are Thunderbird or Seamonkey still using VMs for build infra?  IIRC we did
> parallel make on gmake in VMs without deadlocking because the VMs are so
> damn slow.

SeaMonkey is, but we probably will be ending up moving away from it one of these years. And yes, SeaMonkey is still using -j3 on our VM's so I would appreciate at the least a way to override this error (or no error to be introduced). We do hit the deadlock once and a while, but it is rare.
Here's a first crack at a fix for this. It doesn't handle other Mozilla applications, but it does work for building Firefox on Windows and OS X. Is putting this logic in client.mk the right approach to fixing this, or is there a better place for it?
Assignee: nobody → jon
Status: NEW → ASSIGNED
Attachment #577158 - Flags: feedback?(khuey)
(In reply to Jon Buckley [:jbuck] from comment #3)
> Created attachment 577158 [details] [diff] [review] [diff] [details] [review]
> Throw an error when parallel building (-jN) on Windows with gmake
> 
> Here's a first crack at a fix for this. It doesn't handle other Mozilla
> applications, but it does work for building Firefox on Windows and OS X. Is
> putting this logic in client.mk the right approach to fixing this, or is
> there a better place for it?

Strictly my opinion, IFF we do this, we should do it in the toplevel Makefile.in not client.mk

client.mk itself is not executed in parallel, and people don't need (and sometimes don't) start with client.mk for builds, but client.mk does always dive into top-level Makefile. Could also [instead] be done in config/rules.mk.

As I said above though, I'm against doing this without an "ignore"/"off" switch.
Comment on attachment 577158 [details] [diff] [review]
Throw an error when parallel building (-jN) on Windows with gmake

Lets put this in browser/build.mk instead.
Attachment #577158 - Flags: feedback?(khuey)
I think we could make this relatively foolproof by erroring iff:
1) You're using gmake
2) On Windows
3) And you have >1 core. (Since this is Windows-only, you can probably check the NUMBER_OF_PROCESSORS env var.)
Attached patch Move checks to browser/build.mk (obsolete) — Splinter Review
This patch moves the parallel build checks to browser/build.mk, and changes the check to NUMBER_OF_PROCESSORS instead.
Attachment #577158 - Attachment is obsolete: true
Attachment #577379 - Flags: review?(khuey)
Comment on attachment 577379 [details] [diff] [review]
Move checks to browser/build.mk

This breaks compiling Firefox with gnuMake on windows when youhave multiple CPUs even when you don't -jN.  Which for example, would break current firefox build machines too.
Attachment #577379 - Flags: feedback-
(In reply to Justin Wood (:Callek) from comment #8)
> Comment on attachment 577379 [details] [diff] [review] [diff] [details] [review]
>  Which for example, would break current firefox build machines too.

Err assumptions presented as fact -- fail.

I *think* Current firefox build machines actually have multiple processors assigned, but I did not verify that assumption. but my f- stands since it also breaks the "don't break for the easy/common case" for many users.

Since the deadlocks do not happen with -j1 or no -j at all.
Comment on attachment 577379 [details] [diff] [review]
Move checks to browser/build.mk

Review of attachment 577379 [details] [diff] [review]:
-----------------------------------------------------------------

We shouldn't break -j1 on multiprocessor machines, even if it doesn't affect the build bots.
Attachment #577379 - Flags: review?(khuey) → review-
Now checks for the presence of -j in MAKEFLAGS. MAKEFLAGS doesn't get expanded on the first pass through the file, so this patch uses a workaround found on the make mailing list to search MAKEFLAGS: http://lists.gnu.org/archive/html/help-make/2009-05/msg00023.html
Attachment #577379 - Attachment is obsolete: true
Attachment #577623 - Flags: review?(khuey)
Just for some further clarification, MAKEFLAGS at that point in the file shows "w --jobserver-fds=3,4 -j", if you pass any -jN option via MOZ_MAKE_FLAGS, eg -j8, -j12. If you do not pass -jN in your MOZ_MAKE_FLAGS, then MAKEFLAGS will be "w".

If we need to test for specific -jN values (if someone does MOZ_MAKE_FLAGS=-j1), the only way I can think of right now is exporting the MOZ_MAKE_FLAGS variable in client.mk, so that browser/build.mk can see exactly what flag was set. Looking through mxr, this *might* be a concern with some of the build configs: http://mxr.mozilla.org/mozilla-central/search?string=MOZ_MAKE_FLAGS
Turns out I am being needlessly paranoid about my first patch :)

If you pass MOZ_MAKE_FLAGS="", MAKEFLAGS="w"
If you pass MOZ_MAKE_FLAGS="-j1", MAKEFLAGS="w"
If you pass MOZ_MAKE_FLAGS="-jN", where N is > 1, MAKEFLAGS="w --jobserver-fds=3,4 -j"
Comment on attachment 577623 [details] [diff] [review]
Allow multi-CPU builds to work with -j1

Review of attachment 577623 [details] [diff] [review]:
-----------------------------------------------------------------

Woo.  Looks good to me.
Attachment #577623 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/0e5dfc784411
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 11 → mozilla11
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: