Bug 76445 - [Qt] build-webkit does not detect the number of CPUs
Summary: [Qt] build-webkit does not detect the number of CPUs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2012-01-17 05:11 PST by Seo Sanghyeon
Modified: 2012-02-28 08:27 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.06 KB, patch)
2012-01-17 20:33 PST, Seo Sanghyeon
ossy: review-
Details | Formatted Diff | Diff
proposed patch (1.33 KB, patch)
2012-02-01 08:27 PST, János Badics
vestbo: review-
vestbo: commit-queue-
Details | Formatted Diff | Diff
proposed patch (1.32 KB, patch)
2012-02-06 03:08 PST, János Badics
no flags Details | Formatted Diff | Diff
proposed patch (1.33 KB, patch)
2012-02-06 03:49 PST, János Badics
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Seo Sanghyeon 2012-01-17 05:11:15 PST
https://bugs.webkit.org/show_bug.cgi?id=41469 for Qt.
Comment 1 Allan Sandfeld Jensen 2012-01-17 06:07:55 PST
Is it really necessary, and how would it work with icecc?

I personally just build with MAKEFLAGS=-j24 Tools/Scripts/build-webkit --qt
Comment 2 Seo Sanghyeon 2012-01-17 06:12:37 PST
1. It is a better default.

2. The patch will set -jN only if --makeargs is absent. This is what other ports do, and I think icecc will work fine.
Comment 3 Seo Sanghyeon 2012-01-17 20:33:53 PST
Created attachment 122866 [details]
Patch
Comment 4 Simon Hausmann 2012-01-18 01:08:06 PST
Nice idea. Tor Arne, Ossy, what do you think?
Comment 5 Csaba Osztrogonác 2012-01-18 01:21:31 PST
Comment on attachment 122866 [details]
Patch

The idea is absolutely good, but now it would overload out buildbot servers. 
We run 3-4 bots on all machine with -jXXX set with MAKEFLAGS environment.

It think we should move forward to GTK way. You can set -jXXX manually 
to add it to WebKitMakeArguments environment variable. If you don't do it, 
numberOfCPUs() is the default value.

Please don't land the fix before we fix our bots.
Comment 6 Csaba Osztrogonác 2012-01-18 01:25:08 PST
(In reply to comment #1)
> Is it really necessary, and how would it work with icecc?
> 
> I personally just build with MAKEFLAGS=-j24 Tools/Scripts/build-webkit --qt

I'm not sure what happens after this patch when MAKEFLAGS is set ...
for example: MAKEFLAGS=-j24 make -j8

Which one is stronger? It would be great if build-webkit will respect MAKEFLAGS too.
Comment 7 Tor Arne Vestbø 2012-01-18 01:30:45 PST
(In reply to comment #6)
> (In reply to comment #1)
> > Is it really necessary, and how would it work with icecc?
> > 
> > I personally just build with MAKEFLAGS=-j24 Tools/Scripts/build-webkit --qt
> 
> I'm not sure what happens after this patch when MAKEFLAGS is set ...
> for example: MAKEFLAGS=-j24 make -j8
> 
> Which one is stronger? It would be great if build-webkit will respect MAKEFLAGS too.

It has to. I'm fine with adding logic pass -j if it's not set anywhere else, but it shouldn't break the setup of people who set MAKEFLAGS and friends in their bash/zshrc, eg.
Comment 8 János Badics 2012-02-01 08:27:22 PST
Created attachment 124954 [details]
proposed patch

Automatically determine the number of CPUs for make only if this -j argument haven't already been specified.
Comment 9 Tor Arne Vestbø 2012-02-01 09:45:01 PST
Comment on attachment 124954 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124954&action=review

good stuff, a few comments:

> Tools/Scripts/webkitdirs.pm:2089
> +    if ($makeargs !~ /-j/i && (!defined $ENV{"MAKEFLAGS"} || ($ENV{"MAKEFLAGS"} !~ /-j/i ))){

Slight style (missing space before {), and the regex will trigger on stuff like --just-print as well. build-webkit uses -j\s*\d+
Comment 10 János Badics 2012-02-06 03:08:54 PST
Created attachment 125608 [details]
proposed patch

Added whitespace character before '{' and modified the regex to match only the desired keywords.
Comment 11 János Badics 2012-02-06 03:49:24 PST
Created attachment 125611 [details]
proposed patch

fixed the second regex too
Comment 12 Eric Seidel (no email) 2012-02-10 10:31:24 PST
Comment on attachment 125611 [details]
proposed patch

Entertaining.  There is also code for this in webkitpy/common/ports.py
Comment 13 Csaba Osztrogonác 2012-02-28 07:53:33 PST
Comment on attachment 125611 [details]
proposed patch

LGTM, r=me.

>(From update of attachment 125611 [details])
>Entertaining.  There is also code for this in webkitpy/common/ports.py
I don't think if it is entartaining, but a necessary change, because 
Tools/Scripts/build-webkit doesn't use webkitpy/common/ports.py ,
but many developers and bots use build-webkit script.
Comment 14 WebKit Review Bot 2012-02-28 08:27:24 PST
Comment on attachment 125611 [details]
proposed patch

Clearing flags on attachment: 125611

Committed r109108: <http://trac.webkit.org/changeset/109108>
Comment 15 WebKit Review Bot 2012-02-28 08:27:30 PST
All reviewed patches have been landed.  Closing bug.