Bug 104434 – Make ninja the default build system for build-webkit --chromium on linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make ninja the default build system for build-webkit --chromium

[Eric Seidel (no email)]

Created attachment 178344 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 178344 [details]
Patch

I've emailed webkit-dev to give folks a chance to scream if they're Chromium contributors and anti-ninja for whatever reason.  I'll land this on Tuesday, once I give folks at least one work-day to reply.

[Daniel Bates]

Comment on attachment 178344 [details]
Patch

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

The patch looks good to me. I noticed some very minor spelling nits. As you mentioned in comment 2, we should wait until next week to land this patch so as to give people an opportunity to digest the webkit-dev email and provide feedback.

> Tools/ChangeLog:3
> +        Make ninja the default build system for build-webkit --chromium

Nit: ninja => Ninja

The word "ninja" appears five more times as a proper noun in this patch.

> Tools/ChangeLog:9
> +        as their default build system instead of the native Xcode/VisualStudio/Make.

Nit: VisualStudio => Visual Studio

> Tools/ChangeLog:10
> +        This change makes ninja the default for developers as well as all chromium webkit bots.

Nit: chromium => Chromium

[Peter Beverloo]

For Chromium for Android, one additional change is required. In the following code in webkitdirs.pm (around line 2520):

======
$result = buildChromiumVisualStudioProject("Source/WebKit/chromium/All.sln", $clean);
} elsif (isChromiumNinja() && !isChromiumAndroid()) {
    $result = buildChromiumNinja("all", $clean, @options);
} elsif (isLinux() || isChromiumAndroid() || isChromiumMacMake()) {
======

The !isChromiumAndroid() check forces Ninja off for build-webkit. It should be fine to remove that :).

[Eric Seidel (no email)]

Created attachment 178688 [details]
Patch

[Eric Seidel (no email)]

I'll press the cq+ button in the morning if I don't here any further complaints from non-PSTers before then.

I'll send a note to webkit-dev when I do and will be around to work out any acidental build breakages.

[Eric Seidel (no email)]

Comment on attachment 178688 [details]
Patch

Bombs away!

[Eric Seidel (no email)]

CCing current Chromium/WebKit gardeners.  I'm here to resolve any issues should they arrise.

[Eric Seidel (no email)]

Comment on attachment 178688 [details]
Patch

Clearing flags on attachment: 178688

Committed r137371: <http://trac.webkit.org/changeset/137371>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104731

[Eric Seidel (no email)]

Reverted r137371 for reason:

Various scripts are not ready for out/ as the build directory, this can't work as written.

Committed r137375: <http://trac.webkit.org/changeset/137375>

[Dirk Pranke]

Note that it doesn't look like ninja is currently working in a webkit checkout, so we don't want this to be the default on windows yet. Hopefully I'll find more time to debug this today.

[Nico Weber]

(In reply to comment #13)
> Note that it doesn't look like ninja is currently working in a webkit checkout, so we don't want this to be the default on windows yet. Hopefully I'll find more time to debug this today.

If ninja isn't working on windows, no DumpRenderTree.exe will be created on windows, and this change is harmless on windows.

[Dirk Pranke]

(In reply to comment #14)
> (In reply to comment #13)
> > Note that it doesn't look like ninja is currently working in a webkit checkout, so we don't want this to be the default on windows yet. Hopefully I'll find more time to debug this today.
> 
> If ninja isn't working on windows, no DumpRenderTree.exe will be created on windows, and this change is harmless on windows.

Huh? Right now build-webkit --chromium on windows will successfully compile using msvs; with this change we'll use ninja and the build will fail.

[Nico Weber]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Note that it doesn't look like ninja is currently working in a webkit checkout, so we don't want this to be the default on windows yet. Hopefully I'll find more time to debug this today.
> > 
> > If ninja isn't working on windows, no DumpRenderTree.exe will be created on windows, and this change is harmless on windows.
> 
> Huh? Right now build-webkit --chromium on windows will successfully compile using msvs; with this change we'll use ninja and the build will fail.

Oh sorry, I thought this was the nrwt bug. Nevermind.

[Nico Weber]

With bug 105498 fixed, I think this is ready for another try, at least on linux and mac.

[Nico Weber]

Created attachment 182476 [details]
Patch

[Nico Weber]

Let's try this, on linux first where ninja and make have the same output directory.

[Eric Seidel (no email)]

Comment on attachment 182476 [details]
Patch

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

OK.

> Tools/Scripts/update-webkit:50
> +my $useNinja = $^O eq "linux";

Does this affect non-chromium?

[Nico Weber]

Comment on attachment 182476 [details]
Patch

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

Thanks!

>> Tools/Scripts/update-webkit:50
>> +my $useNinja = $^O eq "linux";
> 
> Does this affect non-chromium?

It will set the environment variable GYP_GENERATORS to "ninja" for all ports. I would think that this has no effect for the other ports.

[WebKit Review Bot]

Comment on attachment 182476 [details]
Patch

Clearing flags on attachment: 182476

Committed r139557: <http://trac.webkit.org/changeset/139557>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

Turns out this didn't change anything on the bots, because Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg says:

class InstallChromiumDependencies(shell.ShellCommand):
    name = "gclient"
    description = ["updating chromium dependencies"]
    descriptionDone = ["updated chromium dependencies"]
    command = ["perl", "./Tools/Scripts/update-webkit-chromium", "--force"]


So the bots always run update-webkit-chromium directly, while we changed update-webkit.

[Nico Weber]

(In reply to comment #24)
> Turns out this didn't change anything on the bots, because Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg says:
> 
> class InstallChromiumDependencies(shell.ShellCommand):
>     name = "gclient"
>     description = ["updating chromium dependencies"]
>     descriptionDone = ["updated chromium dependencies"]
>     command = ["perl", "./Tools/Scripts/update-webkit-chromium", "--force"]
> 
> 
> So the bots always run update-webkit-chromium directly, while we changed update-webkit.

This was added in bug 31442 by yaar@, who's no longer working on chromium. Mark Rowe pointed out that calling update-webkit --chromium might be a better idea on the review; not sure why that wasn't done.

[Nico Weber]

Created attachment 182478 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 182478 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 182478 [details]
Patch

Clearing flags on attachment: 182478

Committed r139559: <http://trac.webkit.org/changeset/139559>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

Let's make this bug about linux. I filed bug 106737 for mac.

[Dirk Pranke]

(In reply to comment #25)
> (In reply to comment #24)
> > Turns out this didn't change anything on the bots, because Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg says:
> > 
> > class InstallChromiumDependencies(shell.ShellCommand):
> >     name = "gclient"
> >     description = ["updating chromium dependencies"]
> >     descriptionDone = ["updated chromium dependencies"]
> >     command = ["perl", "./Tools/Scripts/update-webkit-chromium", "--force"]
> > 
> > 
> > So the bots always run update-webkit-chromium directly, while we changed update-webkit.
> 
> This was added in bug 31442 by yaar@, who's no longer working on chromium. Mark Rowe pointed out that calling update-webkit --chromium might be a better idea on the review; not sure why that wasn't done.

I''ve seen cases where calling update-webkit --chromium doesn't work on windows (it fails to fork the subprocess); I can't recall at the moment if this is win32, msys, or cygwin, and I always figured it was something in my local setup but I haven't dug into it. Perhaps yaar was seeing something similar.