152664 2016-01-03 09:09:53 -0800 [CMake] JSC shell sources should include JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES 2016-01-07 11:30:00 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Minor --- 1 annulen webkit-unassigned achristensen commit-queue gyuyoung.kim lforschler ossy oldest_to_newest 1152109 0 annulen 2016-01-03 09:09:53 -0800 JSC/CMskeLists.txt includes both JavaScriptCore_INCLUDE_DIRECTORIES and JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES, and JSC/shell/CMakeLists.txt should do it as well. In my build setup ICU include paths happen to be in "system" include path, so JSC library builds fine and shells don't 1152114 1 268140 annulen 2016-01-03 09:27:22 -0800 Created attachment 268140 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) 1152193 2 268140 gyuyoung.kim 2016-01-03 17:07:00 -0800 Comment on attachment 268140 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) LGTM. 1152201 3 268140 mcatanzaro 2016-01-03 17:41:27 -0800 Comment on attachment 268140 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) Sorry for breaking this! I am going to give cq- since this same construct exists in PlatformWin.cmake in this directory. Please fix it there too! 1152225 4 gyuyoung.kim 2016-01-03 19:10:39 -0800 (In reply to comment #3) > Comment on attachment 268140 [details] > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > Sorry for breaking this! > > I am going to give cq- since this same construct exists in PlatformWin.cmake > in this directory. Please fix it there too! Do you mean that PlatformWin.cmake includes ${JavaScriptCore_INCLUDE_DIRECTORIES} ? http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/shell/PlatformWin.cmake#L1 If so, I agree to remove it in PlatformWin.cmake although I don't know this patch is tightly related to remove it. 1152282 5 annulen 2016-01-04 02:27:28 -0800 (In reply to comment #3) > Comment on attachment 268140 [details] > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > Sorry for breaking this! > > I am going to give cq- since this same construct exists in PlatformWin.cmake > in this directory. Please fix it there too! I'm not building for Windows, but for me it seems like it includes ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt and second time in PlatformWin.cmake. Other port don't do it 1152658 6 gyuyoung.kim 2016-01-04 22:33:21 -0800 (In reply to comment #5) > (In reply to comment #3) > > Comment on attachment 268140 [details] > > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > > > Sorry for breaking this! > > > > I am going to give cq- since this same construct exists in PlatformWin.cmake > > in this directory. Please fix it there too! > > I'm not building for Windows, but for me it seems like it includes > ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt > and second time in PlatformWin.cmake. Other port don't do it yes, we meant it needs to be fixed in this patch together. 1152690 7 annulen 2016-01-05 02:01:33 -0800 (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #3) > > > Comment on attachment 268140 [details] > > > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > > > > > Sorry for breaking this! > > > > > > I am going to give cq- since this same construct exists in PlatformWin.cmake > > > in this directory. Please fix it there too! > > > > I'm not building for Windows, but for me it seems like it includes > > ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt > > and second time in PlatformWin.cmake. Other port don't do it > > yes, we meant it needs to be fixed in this patch together. Shouldn't JavaScriptCore_INCLUDE_DIRECTORIES be removed from PlatformWin.cmake instead? 1153070 8 achristensen 2016-01-06 10:16:53 -0800 (In reply to comment #7) > Shouldn't JavaScriptCore_INCLUDE_DIRECTORIES be removed from > PlatformWin.cmake instead? No, it's needed because in PlatformWin.cmake we make jscLib, which isn't needed in non-windows ports. jsc.exe is just a simple program that finds jscLib and starts it after possibly finding some other dlls. I don't think it's critical to include JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES on Windows. It wouldn't hurt, but I don't think it's needed. Commit this adding it to Windows or not, but don't remove JavaScriptCore_INCLUDE_DIRECTORIES from PlatformWin.cmake. 1153428 9 268456 annulen 2016-01-07 09:34:29 -0800 Created attachment 268456 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) 1153477 10 268456 commit-queue 2016-01-07 11:29:56 -0800 Comment on attachment 268456 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) Clearing flags on attachment: 268456 Committed r194705: <http://trac.webkit.org/changeset/194705> 1153478 11 commit-queue 2016-01-07 11:30:00 -0800 All reviewed patches have been landed. Closing bug. 268140 2016-01-03 09:27:22 -0800 2016-01-07 09:34:29 -0800 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) 0001-CMake-JSC-shell-sources-should-include-JavaScriptCor.patch text/plain 1844 annulen RnJvbSAxNTU4NDU5YTkwOGVhYTE1NWE5ZGZmMGJlMTZiYzAxMDc3NjEzYWRhIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLb25zdGFudGluIFRva2FyZXYgPGFubnVsZW5AeWFuZGV4LnJ1 PgpEYXRlOiBTdW4sIDMgSmFuIDIwMTYgMjA6MjA6MjggKzAzMDAKU3ViamVjdDogW1BBVENIIDEv MV0gW0NNYWtlXSBKU0Mgc2hlbGwgc291cmNlcyBzaG91bGQgaW5jbHVkZQogSmF2YVNjcmlwdENv cmVfU1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVMKCkpTQy9DTWFrZUxpc3RzLnR4dCBpbmNsdWRl cyBib3RoIEphdmFTY3JpcHRDb3JlX0lOQ0xVREVfRElSRUNUT1JJRVMgYW5kCkphdmFTY3JpcHRD b3JlX1NZU1RFTV9JTkNMVURFX0RJUkVDVE9SSUVTLCBhbmQgSlNDL3NoZWxsL0NNYWtlTGlzdHMu dHh0CnNob3VsZCBkbyBpdCBhcyB3ZWxsLgoKQ2hhbmdlLUlkOiBJZTA5ZjM1YjQzY2ViMjU5ZmZi OWE5YjQ2MzE5YjYwMDU5MGM4NDYwMAotLS0KIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VM b2cgICAgICAgICAgICB8IDkgKysrKysrKysrCiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvc2hlbGwv Q01ha2VMaXN0cy50eHQgfCAxICsKIDIgZmlsZXMgY2hhbmdlZCwgMTAgaW5zZXJ0aW9ucygrKQoK ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDFmNDQ5MjAuLjM4YTkxMGYgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDE2LTAxLTAzICBLb25zdGFudGluIFRv a2FyZXYgIDxhbm51bGVuQHlhbmRleC5ydT4KKworICAgICAgICBbQ01ha2VdIEpTQyBzaGVsbCBz b3VyY2VzIHNob3VsZCBpbmNsdWRlIEphdmFTY3JpcHRDb3JlX1NZU1RFTV9JTkNMVURFX0RJUkVD VE9SSUVTCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x NTI2NjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IHNoZWxsL0NNYWtlTGlzdHMudHh0OgorCiAyMDE2LTAxLTAzICBBbmRyZWFzIEtsaW5nICA8YWts aW5nQGFwcGxlLmNvbT4KIAogICAgICAgICBSZW1vdmUgcmVkdW5kYW50IFN0cmluZ0ltcGwgc3Vi c3RyaW5nIGNyZWF0aW9uIGZ1bmN0aW9uLgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRD b3JlL3NoZWxsL0NNYWtlTGlzdHMudHh0IGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3NoZWxsL0NN YWtlTGlzdHMudHh0CmluZGV4IDU2YzA3MmQuLjNhZGY4ZWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9zaGVsbC9DTWFrZUxpc3RzLnR4dAorKysgYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvc2hlbGwvQ01ha2VMaXN0cy50eHQKQEAgLTIwLDYgKzIwLDcgQEAgV0VCS0lUX0lOQ0xV REVfQ09ORklHX0ZJTEVTX0lGX0VYSVNUUygpCiAKIFdFQktJVF9XUkFQX1NPVVJDRUxJU1QoJHtK U0NfU09VUkNFU30pCiBpbmNsdWRlX2RpcmVjdG9yaWVzKC4vICR7SmF2YVNjcmlwdENvcmVfSU5D TFVERV9ESVJFQ1RPUklFU30pCitpbmNsdWRlX2RpcmVjdG9yaWVzKFNZU1RFTSAke0phdmFTY3Jp cHRDb3JlX1NZU1RFTV9JTkNMVURFX0RJUkVDVE9SSUVTfSkKIGFkZF9leGVjdXRhYmxlKGpzYyAk e0pTQ19TT1VSQ0VTfSkKIHRhcmdldF9saW5rX2xpYnJhcmllcyhqc2MgJHtKU0NfTElCUkFSSUVT fSkKIHNldF90YXJnZXRfcHJvcGVydGllcyhqc2MgUFJPUEVSVElFUyBGT0xERVIgIkphdmFTY3Jp cHRDb3JlIikKLS0gCjEuOS4xCgo= 268456 2016-01-07 09:34:29 -0800 2016-01-07 11:29:56 -0800 Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) 0001-CMake-JSC-shell-sources-should-include-JavaScriptCor.patch text/plain 2372 annulen RnJvbSA2MDZlNWI5YjdmNDQ3Y2M0ZDQ2NGVhMjEyNTZlNTNkYjE4NzQ5NzBmIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLb25zdGFudGluIFRva2FyZXYgPGFubnVsZW5AeWFuZGV4LnJ1 PgpEYXRlOiBTdW4sIDMgSmFuIDIwMTYgMjA6MjA6MjggKzAzMDAKU3ViamVjdDogW1BBVENIIDEv MV0gW0NNYWtlXSBKU0Mgc2hlbGwgc291cmNlcyBzaG91bGQgaW5jbHVkZQogSmF2YVNjcmlwdENv cmVfU1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVMKCkpTQy9DTWFrZUxpc3RzLnR4dCBpbmNsdWRl cyBib3RoIEphdmFTY3JpcHRDb3JlX0lOQ0xVREVfRElSRUNUT1JJRVMgYW5kCkphdmFTY3JpcHRD b3JlX1NZU1RFTV9JTkNMVURFX0RJUkVDVE9SSUVTLCBhbmQgSlNDL3NoZWxsL0NNYWtlTGlzdHMu dHh0CnNob3VsZCBkbyBpdCBhcyB3ZWxsLgoKQ2hhbmdlLUlkOiBJZTA5ZjM1YjQzY2ViMjU5ZmZi OWE5YjQ2MzE5YjYwMDU5MGM4NDYwMAotLS0KIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VM b2cgICAgICAgICAgICAgICB8IDkgKysrKysrKysrCiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvc2hl bGwvQ01ha2VMaXN0cy50eHQgICAgfCAxICsKIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9zaGVsbC9Q bGF0Zm9ybVdpbi5jbWFrZSB8IDEgKwogMyBmaWxlcyBjaGFuZ2VkLCAxMSBpbnNlcnRpb25zKCsp CgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKaW5kZXggODIyMmU5Ni4uMjUzYTlmMCAxMDA2NDQKLS0t IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTYtMDEtMDMgIEtvbnN0YW50aW4g VG9rYXJldiAgPGFubnVsZW5AeWFuZGV4LnJ1PgorCisgICAgICAgIFtDTWFrZV0gSlNDIHNoZWxs IHNvdXJjZXMgc2hvdWxkIGluY2x1ZGUgSmF2YVNjcmlwdENvcmVfU1lTVEVNX0lOQ0xVREVfRElS RUNUT1JJRVMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE1MjY2NAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg ICogc2hlbGwvQ01ha2VMaXN0cy50eHQ6CisKIDIwMTYtMDEtMDcgIEp1bGllbiBCcmlhbmNlYXUg IDxqYnJpYW5jZUBjaXNjby5jb20+CiAKICAgICAgICAgW21pcHNdIEFkZCB0d28gbWlzc2luZyBh Ym9ydFdpdGhSZWFzb24gaW1wbGVtZW50YXRpb25zCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvc2hlbGwvQ01ha2VMaXN0cy50eHQgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvc2hl bGwvQ01ha2VMaXN0cy50eHQKaW5kZXggNTZjMDcyZC4uM2FkZjhlZiAxMDA2NDQKLS0tIGEvU291 cmNlL0phdmFTY3JpcHRDb3JlL3NoZWxsL0NNYWtlTGlzdHMudHh0CisrKyBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9zaGVsbC9DTWFrZUxpc3RzLnR4dApAQCAtMjAsNiArMjAsNyBAQCBXRUJLSVRf SU5DTFVERV9DT05GSUdfRklMRVNfSUZfRVhJU1RTKCkKIAogV0VCS0lUX1dSQVBfU09VUkNFTElT VCgke0pTQ19TT1VSQ0VTfSkKIGluY2x1ZGVfZGlyZWN0b3JpZXMoLi8gJHtKYXZhU2NyaXB0Q29y ZV9JTkNMVURFX0RJUkVDVE9SSUVTfSkKK2luY2x1ZGVfZGlyZWN0b3JpZXMoU1lTVEVNICR7SmF2 YVNjcmlwdENvcmVfU1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVN9KQogYWRkX2V4ZWN1dGFibGUo anNjICR7SlNDX1NPVVJDRVN9KQogdGFyZ2V0X2xpbmtfbGlicmFyaWVzKGpzYyAke0pTQ19MSUJS QVJJRVN9KQogc2V0X3RhcmdldF9wcm9wZXJ0aWVzKGpzYyBQUk9QRVJUSUVTIEZPTERFUiAiSmF2 YVNjcmlwdENvcmUiKQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3NoZWxsL1Bs YXRmb3JtV2luLmNtYWtlIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3NoZWxsL1BsYXRmb3JtV2lu LmNtYWtlCmluZGV4IDAzMDQ1OTUuLmRmZTY4ZjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9zaGVsbC9QbGF0Zm9ybVdpbi5jbWFrZQorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvc2hlbGwvUGxhdGZvcm1XaW4uY21ha2UKQEAgLTEsNCArMSw1IEBACiBpbmNsdWRlX2RpcmVj dG9yaWVzKC4vICR7SmF2YVNjcmlwdENvcmVfSU5DTFVERV9ESVJFQ1RPUklFU30pCitpbmNsdWRl X2RpcmVjdG9yaWVzKFNZU1RFTSAke0phdmFTY3JpcHRDb3JlX1NZU1RFTV9JTkNMVURFX0RJUkVD VE9SSUVTfSkKIGFkZF9saWJyYXJ5KGpzY0xpYiBTSEFSRUQgJHtKU0NfU09VUkNFU30pCiAKIGxp c3QoQVBQRU5EIEpTQ19MSUJSQVJJRVMKLS0gCjEuOS4xCgo=