177057 2017-09-17 14:02:35 -0700 build-webkit spawns fewer subprocesses than ninja uses by default 2017-09-27 12:26:32 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 177223 1 thorton thorton achristensen aestes annulen ap clopez commit-queue ggaren keith_miller webkit-bug-importer oldest_to_newest 1349788 0 thorton 2017-09-17 14:02:35 -0700 build-webkit spawns fewer subprocesses than ninja uses by default 1349789 1 321054 thorton 2017-09-17 14:02:55 -0700 Created attachment 321054 Patch 1349794 2 321054 ap 2017-09-17 14:56:14 -0700 Comment on attachment 321054 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review > Tools/Scripts/build-webkit:245 > + # so don't override the number of jobs if building with Ninja. Would this number be better when not building with ninja, too? The comment should at least explain why (e.g. “I didn’t test, and didn’t want to make the change blindly”). 1349795 3 321054 thorton 2017-09-17 14:57:48 -0700 Comment on attachment 321054 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review >> Tools/Scripts/build-webkit:245 >> + # so don't override the number of jobs if building with Ninja. > > Would this number be better when not building with ninja, too? The comment should at least explain why (e.g. “I didn’t test, and didn’t want to make the change blindly”). I considered it, and would happily make the change blindly, but I’m also not sure anybody cares about those builds (it won’t affect Xcode, for example — just non-Windows non-Ninja CMake builds). 1349894 4 321054 achristensen 2017-09-18 08:13:12 -0700 Comment on attachment 321054 Patch We should only make this change for ninja, which automatically determines the number of jobs to do at the same time. If we do this for non-ninja builds, it would make them take ~8x as long because make defaults to using 1 cpu. 1349895 5 thorton 2017-09-18 08:17:09 -0700 I think (at least I assume) ap meant adding the “+2” to the make invocation 1349923 6 ap 2017-09-18 09:20:17 -0700 Correct. 1349947 7 321054 commit-queue 2017-09-18 09:54:47 -0700 Comment on attachment 321054 Patch Clearing flags on attachment: 321054 Committed r222160: <http://trac.webkit.org/changeset/222160> 1349948 8 commit-queue 2017-09-18 09:54:49 -0700 All reviewed patches have been landed. Closing bug. 1350837 9 321054 annulen 2017-09-20 10:43:24 -0700 Comment on attachment 321054 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review > Tools/Scripts/build-webkit:242 > if (isCMakeBuild() && !isAnyWindows()) { > + if (!canUseNinja()) { This condition is wrong, $willUseNinja in generateBuildSystemFromCMakeProject() determines what cmake generator is used, and it's defined as canUseNinja() && canUseNinjaGenerator() 1350839 10 annulen 2017-09-20 10:45:25 -0700 Though canUseNinjaGenerator can be eleminated, as all required cmake versions should support -G Ninja 1350840 11 thorton 2017-09-20 10:50:30 -0700 (In reply to Konstantin Tokarev from comment #10) > Though canUseNinjaGenerator can be eleminated, as all required cmake > versions should support -G Ninja Doesn't that then make the patch right? :P 1350848 12 annulen 2017-09-20 11:00:11 -0700 Yes, it does. CMake enables Ninja generator on all supported platforms since v2.8.9 (https://github.com/Kitware/CMake/commit/5d365b26ec6ce089f1a5e0bfed523cb5f916f1da) and we require v3.3 1350868 13 thorton 2017-09-20 11:23:59 -0700 Nice 1351454 14 thorton 2017-09-21 16:29:37 -0700 Got rid of it in bug 177330 1351593 15 annulen 2017-09-22 03:15:42 -0700 Thanks! 1352162 16 clopez 2017-09-25 03:33:01 -0700 (In reply to WebKit Commit Bot from comment #7) > Comment on attachment 321054 [details] > Patch > > Clearing flags on attachment: 321054 > > Committed r222160: <http://trac.webkit.org/changeset/222160> This patch has broken our bots. We need to control the number of build parallel process there as we share the machine between several bots (each one run in a container). And if we allow all of them to use all the available cores bad things happen. See bug 177223 I think that when the environment variable NUMBER_OF_PROCESSORS is defined it should be respected (both for ninja and make). I will try to upload a patch to bug 177223 ASAP 1353383 17 webkit-bug-importer 2017-09-27 12:26:32 -0700 <rdar://problem/34693285> 321054 2017-09-17 14:02:55 -0700 2017-09-18 09:54:47 -0700 Patch bug-177057-20170917140254.patch text/plain 1793 thorton SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIyMjEzNykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDE3LTA5LTE3ICBUaW0gSG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUu Y29tPgorCisgICAgICAgIGJ1aWxkLXdlYmtpdCBzcGF3bnMgZmV3ZXIgc3VicHJvY2Vzc2VzIHRo YW4gbmluamEgdXNlcyBieSBkZWZhdWx0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNzcwNTcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIFNjcmlwdHMvYnVpbGQtd2Via2l0OgorICAgICAgICBCeSBkZWZh dWx0LCBuaW5qYSB3aWxsIHBhcmFsbGVsaXplIG92ZXIgKGNvcmVzICsgMikgam9icy4gYnVpbGQt d2Via2l0CisgICAgICAgIHNwZWNpZmllcyAtaihjb3JlcyksIHdoaWNoIG92ZXJyaWRlcyB0aGlz LiBSZW1vdmUgb3VyIG92ZXJyaWRlIGlmIGJ1aWxkaW5nCisgICAgICAgIHdpdGggbmluamE7IGp1 c3QgbGV0IGl0IGRvIGl0cyBvd24gdGhpbmcuIEluIG15IHRlc3RpbmcsIHRoaXMgbWlub3IKKyAg ICAgICAgY2hhbmdlIGlzIHRoZSBkaWZmZXJlbmNlIGJldHdlZW4gfjEwJSBpZGxlIENQVSB0aW1l IGFuZCAwLgorCiAyMDE3LTA5LTE3ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29t PgogCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzcw MzgKSW5kZXg6IFRvb2xzL1NjcmlwdHMvYnVpbGQtd2Via2l0Cj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xz L1NjcmlwdHMvYnVpbGQtd2Via2l0CShyZXZpc2lvbiAyMjIxMzcpCisrKyBUb29scy9TY3JpcHRz L2J1aWxkLXdlYmtpdAkod29ya2luZyBjb3B5KQpAQCAtMjM5LDkgKzIzOSwxMyBAQCBpZiAoaXNJ bnNwZWN0b3JGcm9udGVuZCgpKSB7CiB9CiAKIGlmIChpc0NNYWtlQnVpbGQoKSAmJiAhaXNBbnlX aW5kb3dzKCkpIHsKKyAgICBpZiAoIWNhblVzZU5pbmphKCkpIHsKKyAgICAgICAgIyBCeSBkZWZh dWx0IHdlIGJ1aWxkIHVzaW5nIGFsbCBvZiB0aGUgYXZhaWxhYmxlIENQVXMuCisgICAgICAgICMg TmluamEgd2lsbCBhdXRvbWF0aWNhbGx5IGRldGVybWluZSB0aGUgbnVtYmVyIG9mIGpvYnMgdG8g cnVuIGluIHBhcmFsbGVsLAorICAgICAgICAjIHNvIGRvbid0IG92ZXJyaWRlIHRoZSBudW1iZXIg b2Ygam9icyBpZiBidWlsZGluZyB3aXRoIE5pbmphLgorICAgICAgICAkbWFrZUFyZ3MgLj0gKCRt YWtlQXJncyA/ICIgIiA6ICIiKSAuICItaiIgLiBudW1iZXJPZkNQVXMoKSBpZiAkbWFrZUFyZ3Mg IX4gLy1qXHMqXGQrLzsKKyAgICB9CiAKLSAgICAjIEJ5IGRlZmF1bHQgd2UgYnVpbGQgdXNpbmcg YWxsIG9mIHRoZSBhdmFpbGFibGUgQ1BVcy4KLSAgICAkbWFrZUFyZ3MgLj0gKCRtYWtlQXJncyA/ ICIgIiA6ICIiKSAuICItaiIgLiBudW1iZXJPZkNQVXMoKSBpZiAkbWFrZUFyZ3MgIX4gLy1qXHMq XGQrLzsKICAgICBteSAkbWF4Q1BVTG9hZCA9IG1heENQVUxvYWQoKSBpZiAkbWFrZUFyZ3MgIX4g Ly1sXHMqXGQrXC4/XGQqLzsKICAgICAkbWFrZUFyZ3MgLj0gIiAtbCIgLiBtYXhDUFVMb2FkKCkg aWYgZGVmaW5lZCAkbWF4Q1BVTG9hZDsKIAo=