237170 2022-02-24 17:34:25 -0800 [CMake] Disabling ENABLE_WEBCORE is ignored when cmake configuration runs again. 2022-03-01 01:38:17 -0800 1 1 1 Unclassified WebKit CMake WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 basuke basuke annulen basuke ews-watchlist fujii gyuyoung.kim ryuan.choi sergio stephan.szabo oldest_to_newest 1845849 0 basuke 2022-02-24 17:34:25 -0800 When I create a project by cmake with -DENABLE_WEBCORE=OFF, it generates a proper project which doesn't build WebCore and above at all. But if I change some CMake related files and it is required to re-run cmake configuration, then it creates a project with the ENABLE_WEBCORE is ON. > cmake -DCMAKE_TOOLCHAIN_FILE=Platform\PlayStation5 -G Ninja -DPORT="PlayStation" -DENABLE_WEBCORE=OFF -DCMAKE_BUILD_TYPE=Release -B WebKitBuild/jsc > cmake --build WebKitBuild/jsc # build is good Then modify CMake files such as PlatformPlayStation.cmake, and re-run build, > cmake --build WebKitBuild/jsc # build is NG, ENABLE_WEBCORE is back to ON I expected the project should use same variables. Unless re-running configuration, the project is stable. I think this variable should be cached in CMake. 1845854 1 basuke 2022-02-24 17:42:59 -0800 This change seems working as I expected, but I'm not sure this is the correct direction. diff --git a/Source/cmake/WebKitCommon.cmake b/Source/cmake/WebKitCommon.cmake index e58c7069abb1..c3389c8e46f7 100644 --- a/Source/cmake/WebKitCommon.cmake +++ b/Source/cmake/WebKitCommon.cmake @@ -13,19 +13,19 @@ if (NOT HAS_RUN_WEBKIT_COMMON) message(STATUS "The CMake build type is: ${CMAKE_BUILD_TYPE}") endif () - set(ENABLE_JAVASCRIPTCORE ON) - set(ENABLE_WEBCORE ON) + set(ENABLE_JAVASCRIPTCORE ON CACHE STRING "Enable JavaScriptCore build") + set(ENABLE_WEBCORE ON ON CACHE STRING "Enable WebCore build") if (NOT DEFINED ENABLE_WEBKIT) - set(ENABLE_WEBKIT ON) + set(ENABLE_WEBKIT ON ON CACHE STRING "Enable WebKit build") endif () if (NOT DEFINED ENABLE_TOOLS AND EXISTS "${CMAKE_SOURCE_DIR}/Tools") - set(ENABLE_TOOLS ON) + set(ENABLE_TOOLS ON ON CACHE STRING "Enable Tools build") endif () if (NOT DEFINED ENABLE_WEBINSPECTORUI) - set(ENABLE_WEBINSPECTORUI ON) + set(ENABLE_WEBINSPECTORUI ON ON CACHE STRING "Enable WebInspector UI build") endif () # ----------------------------------------------------------------------------- 1845867 2 fujii 2022-02-24 18:10:40 -0800 If you want to change ENABLE_WEBCORE from outside, using option is the CMake way in such case. https://cmake.org/cmake/help/latest/command/option.html 1845868 3 stephan.szabo 2022-02-24 18:12:46 -0800 It seems like it'd work better if those were cache variables rather than normal ones if we're expecting them to have overwritten values from the command line that'll persist. The others are using an if(NOT DEFINED XXX) block, which may also work if put around the set in WebKitCommon, but seems like it's just doing the same sort of thing that a cache variable would already do. Is there some piece of the cache variable behavior that's problematic for what we're doing (for example maybe around the HAS_RUN_WEBKIT_COMMON bits?) 1846770 4 453425 basuke 2022-02-28 14:12:38 -0800 Created attachment 453425 PATCH 1846901 5 453425 fujii 2022-02-28 20:19:56 -0800 Comment on attachment 453425 PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=453425&action=review > Source/cmake/WebKitCommon.cmake:18 > + option(ENABLE_WEBKIT "Enable building WebKit" ON) Some ports set these values OFF by default. It'd be nice if we can change the default value of these options. I think there two ways to do so. 1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake. 2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT. But, it seemingly needs some refactoring. I don't object landing this patch if you want. 1846909 6 basuke 2022-02-28 21:06:14 -0800 (In reply to Fujii Hironori from comment #5) > Comment on attachment 453425 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453425&action=review > > > Source/cmake/WebKitCommon.cmake:18 > > + option(ENABLE_WEBKIT "Enable building WebKit" ON) > > Some ports set these values OFF by default. > It'd be nice if we can change the default value of these options. > I think there two ways to do so. > 1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake. > 2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT. > But, it seemingly needs some refactoring. I don't object landing this patch > if you want. Correct. While touching the code, I was fighting with the attempt of refactoring more and more. Then ended up to just doing the straight-forward thing right now. As you say, I'll land this right now and do more refactoring later. 1846937 7 453478 basuke 2022-03-01 00:23:10 -0800 Created attachment 453478 PATCH 1846950 8 ews-feeder 2022-03-01 01:38:12 -0800 Committed r290636 (247909@main): <https://commits.webkit.org/247909@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453478. 453425 2022-02-28 14:12:38 -0800 2022-03-01 00:23:10 -0800 PATCH patch.diff text/plain 2704 basuke ZGlmZiAtLWdpdCBhL0NoYW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCA0NmI0MmMzYWMwZGMuLjE1 MzFiYmY2YTY2ZiAxMDA2NDQKLS0tIGEvQ2hhbmdlTG9nCisrKyBiL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDIwIEBACisyMDIyLTAyLTI4ICBCYXN1a2UgU3V6dWtpICA8YmFzdWtlLnN1enVraUBzb255 LmNvbT4KKworICAgICAgICBbQ01ha2VdIERpc2FibGluZyBFTkFCTEVfV0VCQ09SRSBpcyBpZ25v cmVkIHdoZW4gY21ha2UgY29uZmlndXJhdGlvbiBydW5zIGFnYWluLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjM3MTcwCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ01ha2UgdmFyaWFibGVzIHdoaWNoIGhh cyBjaGFuY2UgdG8gc2V0IGZyb20gb3V0c2lkZSBzaG91bGQgYmUgY2FjaGVkIGluIENNYWtlIGNv bmZpZ3VyYXRpb24gY2FjaGUuCisgICAgICAgIFVubGVzcyBjYWNoZWluZywgdGhlIHJlc3VsdCBv ZiBidWlsZGluZyB0aGUgZ2VuZXJhdGVkIHByb2plY3QgaXMgbm90IGNvbnNpc2l0ZW50IGJlY2F1 c2UgdGhlIG90aGVyCisgICAgICAgIGNvbmZpZ3VyYXRpb24gbWF5IHJ1biB3aGlsZSBidWlsZGlu Zy4KKworICAgICAgICBGb3IgY29tcGxpY2F0aW9uLCBJJ3ZlIGNoYW5nZWQgRU5BQkxFX0pBVkFT Q1JJUFRDT1JFLCBFTkFCTEVfV0VCQ09SRSBhbmQgRU5BQkxFX1dFQktJVCB0byBgb3B0aW9uKClg CisgICAgICAgIHdoaWNoIGlzIHN0b3JlZCBpbiBjYWNoZWQgYW5kIHJldXNlZCB3aGlsZSBidWls ZGluZyBldmVuIGlmIHRoZSBjb25mdWd1cmF0aW9uIHJ1bnMgYWdhaW4uCisKKyAgICAgICAgKiBT b3VyY2UvY21ha2UvT3B0aW9uc1BsYXlTdGF0aW9uLmNtYWtlOgorICAgICAgICAqIFNvdXJjZS9j bWFrZS9XZWJLaXRDb21tb24uY21ha2U6CisKIDIwMjItMDItMjUgIFphbiBEb2JlcnNlayAgPHpk b2JlcnNla0BpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdW1dQRV0gVW5jb3VwbGUgbGliZ2Jt LCBsaWJkcm0gZGVwZW5kZW5jaWVzIGZyb20gQU5HTEUgZnVuY3Rpb25hbGl0eQpkaWZmIC0tZ2l0 IGEvU291cmNlL2NtYWtlL09wdGlvbnNQbGF5U3RhdGlvbi5jbWFrZSBiL1NvdXJjZS9jbWFrZS9P cHRpb25zUGxheVN0YXRpb24uY21ha2UKaW5kZXggYjAyMzFlZjk3ZTY0Li5lMWM5OWM4YWE4ZDkg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9jbWFrZS9PcHRpb25zUGxheVN0YXRpb24uY21ha2UKKysrIGIv U291cmNlL2NtYWtlL09wdGlvbnNQbGF5U3RhdGlvbi5jbWFrZQpAQCAtMTcsMTUgKzE3LDkgQEAg YWRkX2RlZmluaXRpb25zKC1EU0NFX0xJQkNfRElTQUJMRV9DUFAxNF9IRUFERVJfV0FSTklORz0g LURTQ0VfTElCQ19ESVNBQkxFX0NQUDEKIFdFQktJVF9QUkVQRU5EX0dMT0JBTF9DT01QSUxFUl9G TEFHUygtV25vLWRsbC1hdHRyaWJ1dGUtb24tcmVkZWNsYXJhdGlvbikKIAogc2V0KEVOQUJMRV9B UElfVEVTVFMgT04gQ0FDSEUgQk9PTCAiQnVpbGQgQVBJIFRlc3RzIikKLXNldChFTkFCTEVfV0VC Q09SRSBPTiBDQUNIRSBCT09MICJCdWlsZCBXZWJDb3JlIikKLXNldChFTkFCTEVfV0VCS0lUIE9O IENBQ0hFIEJPT0wgIkJ1aWxkIFdlYktpdCIpCiBzZXQoRU5BQkxFX1dFQktJVF9MRUdBQ1kgT0ZG KQogc2V0KEVOQUJMRV9XRUJJTlNQRUNUT1JVSSBPRkYpCiAKLWlmIChOT1QgRU5BQkxFX1dFQkNP UkUpCi0gICAgc2V0KEVOQUJMRV9XRUJLSVQgT0ZGKQotZW5kaWYgKCkKLQogV0VCS0lUX09QVElP Tl9CRUdJTigpCiAKICMgUGxheVN0YXRpb24gU3BlY2lmaWMgT3B0aW9ucwpkaWZmIC0tZ2l0IGEv U291cmNlL2NtYWtlL1dlYktpdENvbW1vbi5jbWFrZSBiL1NvdXJjZS9jbWFrZS9XZWJLaXRDb21t b24uY21ha2UKaW5kZXggZTU4YzcwNjlhYmIxLi4zMWJiOTU3ZDRkNDggMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9jbWFrZS9XZWJLaXRDb21tb24uY21ha2UKKysrIGIvU291cmNlL2NtYWtlL1dlYktpdENv bW1vbi5jbWFrZQpAQCAtMTMsMTEgKzEzLDE2IEBAIGlmIChOT1QgSEFTX1JVTl9XRUJLSVRfQ09N TU9OKQogICAgICAgICBtZXNzYWdlKFNUQVRVUyAiVGhlIENNYWtlIGJ1aWxkIHR5cGUgaXM6ICR7 Q01BS0VfQlVJTERfVFlQRX0iKQogICAgIGVuZGlmICgpCiAKLSAgICBzZXQoRU5BQkxFX0pBVkFT Q1JJUFRDT1JFIE9OKQotICAgIHNldChFTkFCTEVfV0VCQ09SRSBPTikKKyAgICBvcHRpb24oRU5B QkxFX0pBVkFTQ1JJUFRDT1JFICJFbmFibGUgYnVpbGRpbmcgSmF2YVNjcmlwdENvcmUiIE9OKQor ICAgIG9wdGlvbihFTkFCTEVfV0VCQ09SRSAiRW5hYmxlIGJ1aWxkaW5nIEphdmFTY3JpcHRDb3Jl IiBPTikKKyAgICBvcHRpb24oRU5BQkxFX1dFQktJVCAiRW5hYmxlIGJ1aWxkaW5nIFdlYktpdCIg T04pCiAKLSAgICBpZiAoTk9UIERFRklORUQgRU5BQkxFX1dFQktJVCkKLSAgICAgICAgc2V0KEVO QUJMRV9XRUJLSVQgT04pCisgICAgaWYgKE5PVCBFTkFCTEVfSkFWQVNDUklQVENPUkUpCisgICAg ICAgIHNldChFTkFCTEVfV0VCQ09SRSBPRkYpCisgICAgZW5kaWYgKCkKKworICAgIGlmIChOT1Qg RU5BQkxFX1dFQkNPUkUpCisgICAgICAgIHNldChFTkFCTEVfV0VCS0lUIE9GRikKICAgICBlbmRp ZiAoKQogCiAgICAgaWYgKE5PVCBERUZJTkVEIEVOQUJMRV9UT09MUyBBTkQgRVhJU1RTICIke0NN QUtFX1NPVVJDRV9ESVJ9L1Rvb2xzIikNCg== 453478 2022-03-01 00:23:10 -0800 2022-03-01 01:38:15 -0800 PATCH patch.diff text/plain 2707 basuke ZGlmZiAtLWdpdCBhL0NoYW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCA0NmI0MmMzYWMwZGMuLjgx YTU1NWU4NTVmNiAxMDA2NDQKLS0tIGEvQ2hhbmdlTG9nCisrKyBiL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDIwIEBACisyMDIyLTAyLTI4ICBCYXN1a2UgU3V6dWtpICA8YmFzdWtlLnN1enVraUBzb255 LmNvbT4KKworICAgICAgICBbQ01ha2VdIERpc2FibGluZyBFTkFCTEVfV0VCQ09SRSBpcyBpZ25v cmVkIHdoZW4gY21ha2UgY29uZmlndXJhdGlvbiBydW5zIGFnYWluLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjM3MTcwCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgRnVqaWkgSGlyb25vcmkuCisKKyAgICAgICAgQ01ha2UgdmFyaWFibGVzIHdoaWNoIGhh cyBjaGFuY2UgdG8gc2V0IGZyb20gb3V0c2lkZSBzaG91bGQgYmUgY2FjaGVkIGluIENNYWtlIGNv bmZpZ3VyYXRpb24gY2FjaGUuCisgICAgICAgIFVubGVzcyBjYWNoZWluZywgdGhlIHJlc3VsdCBv ZiBidWlsZGluZyB0aGUgZ2VuZXJhdGVkIHByb2plY3QgaXMgbm90IGNvbnNpc2l0ZW50IGJlY2F1 c2UgdGhlIG90aGVyCisgICAgICAgIGNvbmZpZ3VyYXRpb24gbWF5IHJ1biB3aGlsZSBidWlsZGlu Zy4KKworICAgICAgICBUbyBtYWtlIGl0IGNvbXBsZXRlLCBJJ3ZlIGNoYW5nZWQgRU5BQkxFX0pB VkFTQ1JJUFRDT1JFLCBFTkFCTEVfV0VCQ09SRSBhbmQgRU5BQkxFX1dFQktJVCB0byBgb3B0aW9u KClgCisgICAgICAgIHdoaWNoIGlzIHN0b3JlZCBpbiBjYWNoZWQgYW5kIHJldXNlZCB3aGlsZSBi dWlsZGluZyBldmVuIGlmIHRoZSBjb25mdWd1cmF0aW9uIHJ1bnMgYWdhaW4uCisKKyAgICAgICAg KiBTb3VyY2UvY21ha2UvT3B0aW9uc1BsYXlTdGF0aW9uLmNtYWtlOgorICAgICAgICAqIFNvdXJj ZS9jbWFrZS9XZWJLaXRDb21tb24uY21ha2U6CisKIDIwMjItMDItMjUgIFphbiBEb2JlcnNlayAg PHpkb2JlcnNla0BpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdW1dQRV0gVW5jb3VwbGUgbGli Z2JtLCBsaWJkcm0gZGVwZW5kZW5jaWVzIGZyb20gQU5HTEUgZnVuY3Rpb25hbGl0eQpkaWZmIC0t Z2l0IGEvU291cmNlL2NtYWtlL09wdGlvbnNQbGF5U3RhdGlvbi5jbWFrZSBiL1NvdXJjZS9jbWFr ZS9PcHRpb25zUGxheVN0YXRpb24uY21ha2UKaW5kZXggYjAyMzFlZjk3ZTY0Li5lMWM5OWM4YWE4 ZDkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9jbWFrZS9PcHRpb25zUGxheVN0YXRpb24uY21ha2UKKysr IGIvU291cmNlL2NtYWtlL09wdGlvbnNQbGF5U3RhdGlvbi5jbWFrZQpAQCAtMTcsMTUgKzE3LDkg QEAgYWRkX2RlZmluaXRpb25zKC1EU0NFX0xJQkNfRElTQUJMRV9DUFAxNF9IRUFERVJfV0FSTklO Rz0gLURTQ0VfTElCQ19ESVNBQkxFX0NQUDEKIFdFQktJVF9QUkVQRU5EX0dMT0JBTF9DT01QSUxF Ul9GTEFHUygtV25vLWRsbC1hdHRyaWJ1dGUtb24tcmVkZWNsYXJhdGlvbikKIAogc2V0KEVOQUJM RV9BUElfVEVTVFMgT04gQ0FDSEUgQk9PTCAiQnVpbGQgQVBJIFRlc3RzIikKLXNldChFTkFCTEVf V0VCQ09SRSBPTiBDQUNIRSBCT09MICJCdWlsZCBXZWJDb3JlIikKLXNldChFTkFCTEVfV0VCS0lU IE9OIENBQ0hFIEJPT0wgIkJ1aWxkIFdlYktpdCIpCiBzZXQoRU5BQkxFX1dFQktJVF9MRUdBQ1kg T0ZGKQogc2V0KEVOQUJMRV9XRUJJTlNQRUNUT1JVSSBPRkYpCiAKLWlmIChOT1QgRU5BQkxFX1dF QkNPUkUpCi0gICAgc2V0KEVOQUJMRV9XRUJLSVQgT0ZGKQotZW5kaWYgKCkKLQogV0VCS0lUX09Q VElPTl9CRUdJTigpCiAKICMgUGxheVN0YXRpb24gU3BlY2lmaWMgT3B0aW9ucwpkaWZmIC0tZ2l0 IGEvU291cmNlL2NtYWtlL1dlYktpdENvbW1vbi5jbWFrZSBiL1NvdXJjZS9jbWFrZS9XZWJLaXRD b21tb24uY21ha2UKaW5kZXggZTU4YzcwNjlhYmIxLi4zMWJiOTU3ZDRkNDggMTAwNjQ0Ci0tLSBh L1NvdXJjZS9jbWFrZS9XZWJLaXRDb21tb24uY21ha2UKKysrIGIvU291cmNlL2NtYWtlL1dlYktp dENvbW1vbi5jbWFrZQpAQCAtMTMsMTEgKzEzLDE2IEBAIGlmIChOT1QgSEFTX1JVTl9XRUJLSVRf Q09NTU9OKQogICAgICAgICBtZXNzYWdlKFNUQVRVUyAiVGhlIENNYWtlIGJ1aWxkIHR5cGUgaXM6 ICR7Q01BS0VfQlVJTERfVFlQRX0iKQogICAgIGVuZGlmICgpCiAKLSAgICBzZXQoRU5BQkxFX0pB VkFTQ1JJUFRDT1JFIE9OKQotICAgIHNldChFTkFCTEVfV0VCQ09SRSBPTikKKyAgICBvcHRpb24o RU5BQkxFX0pBVkFTQ1JJUFRDT1JFICJFbmFibGUgYnVpbGRpbmcgSmF2YVNjcmlwdENvcmUiIE9O KQorICAgIG9wdGlvbihFTkFCTEVfV0VCQ09SRSAiRW5hYmxlIGJ1aWxkaW5nIEphdmFTY3JpcHRD b3JlIiBPTikKKyAgICBvcHRpb24oRU5BQkxFX1dFQktJVCAiRW5hYmxlIGJ1aWxkaW5nIFdlYktp dCIgT04pCiAKLSAgICBpZiAoTk9UIERFRklORUQgRU5BQkxFX1dFQktJVCkKLSAgICAgICAgc2V0 KEVOQUJMRV9XRUJLSVQgT04pCisgICAgaWYgKE5PVCBFTkFCTEVfSkFWQVNDUklQVENPUkUpCisg ICAgICAgIHNldChFTkFCTEVfV0VCQ09SRSBPRkYpCisgICAgZW5kaWYgKCkKKworICAgIGlmIChO T1QgRU5BQkxFX1dFQkNPUkUpCisgICAgICAgIHNldChFTkFCTEVfV0VCS0lUIE9GRikKICAgICBl bmRpZiAoKQogCiAgICAgaWYgKE5PVCBERUZJTkVEIEVOQUJMRV9UT09MUyBBTkQgRVhJU1RTICIk e0NNQUtFX1NPVVJDRV9ESVJ9L1Rvb2xzIikNCg==