[Qt] build-webkit does not detect the number of CPUs

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Seo Sanghyeon]

https://bugs.webkit.org/show_bug.cgi?id=41469 for Qt.

[Allan Sandfeld Jensen]

Is it really necessary, and how would it work with icecc?

I personally just build with MAKEFLAGS=-j24 Tools/Scripts/build-webkit --qt

[Seo Sanghyeon]

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.

[Seo Sanghyeon]

Created attachment 122866 [details]
Patch

[Simon Hausmann]

Nice idea. Tor Arne, Ossy, what do you think?

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Tor Arne Vestbø]

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

[János Badics]

Created attachment 124954 [details]
proposed patch

Automatically determine the number of CPUs for make only if this -j argument haven't already been specified.

[Tor Arne Vestbø]

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+

[János Badics]

Created attachment 125608 [details]
proposed patch

Added whitespace character before '{' and modified the regex to match only the desired keywords.

[János Badics]

Created attachment 125611 [details]
proposed patch

fixed the second regex too

[Eric Seidel (no email)]

Comment on attachment 125611 [details]
proposed patch

Entertaining.  There is also code for this in webkitpy/common/ports.py

[Csaba Osztrogonác]

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.

[WebKit Review Bot]

Comment on attachment 125611 [details]
proposed patch

Clearing flags on attachment: 125611

Committed r109108: <http://trac.webkit.org/changeset/109108>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.