Make ninja the default build system for build-webkit --chromium
Created attachment 178344 [details] Patch
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.
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
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 :).
Created attachment 178688 [details] Patch
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.
Comment on attachment 178688 [details] Patch Bombs away!
CCing current Chromium/WebKit gardeners. I'm here to resolve any issues should they arrise.
Comment on attachment 178688 [details] Patch Clearing flags on attachment: 178688 Committed r137371: <http://trac.webkit.org/changeset/137371>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 104731
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>
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.
(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.
(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.
(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.
With bug 105498 fixed, I think this is ready for another try, at least on linux and mac.
Created attachment 182476 [details] Patch
Let's try this, on linux first where ninja and make have the same output directory.
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?
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.
Comment on attachment 182476 [details] Patch Clearing flags on attachment: 182476 Committed r139557: <http://trac.webkit.org/changeset/139557>
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.
(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.
Created attachment 182478 [details] Patch
Comment on attachment 182478 [details] Patch OK.
Comment on attachment 182478 [details] Patch Clearing flags on attachment: 182478 Committed r139559: <http://trac.webkit.org/changeset/139559>
Let's make this bug about linux. I filed bug 106737 for mac.
(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.