158727 2016-06-14 00:25:22 -0700 [Win][CMake] Changes in WebKit options are not reflected in incremental builds. 2016-06-15 10:06:34 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pvollan pvollan achristensen bfulgham commit-queue dbates fujii lforschler oldest_to_newest 1202115 0 pvollan 2016-06-14 00:25:22 -0700 Changing one of the WebKit options in OptionsWin.cmake, have no effect on incremental builds. This happens because the CMake option command only sets an initial value. Incremental builds will use the cached value. 1202194 1 281261 pvollan 2016-06-14 08:36:35 -0700 Created attachment 281261 Patch 1202195 2 pvollan 2016-06-14 08:43:10 -0700 I am not convinced that this is the best fix for this, since it creates a "normal" CMake variable instead of a persistent, cached CMake variable. It might cause other problems. Another option is to create a cached variable with 'set' and force update the cache: set(${_name} ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}} CACHE BOOL "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}" FORCE) I believe this will be the same as using 'option', except that the CMake cache will be updated. 1202196 3 achristensen 2016-06-14 09:04:43 -0700 I think GTK and EFL are using shouldRemoveCMakeCache is webkitdirs.pm. Are we using that on Windows? 1202506 4 fujii 2016-06-14 23:50:39 -0700 I think you need to check the variable is already defined before setting. > if (DEFINED ${_name}) Because the variable can be given like cmake -D ENABLE_MATHML=OFF if I invoke build-webkit --no-mathml. This change affects developers who don't use build-webkit for incremental builds like me. In incremental builds, cmake will be run automatically by invoking ninja or Visual Studio's build command if CMake scripts have been modified. In this time, build options will be restored from CMakeCache.txt. 1202515 5 281342 pvollan 2016-06-15 00:22:25 -0700 Created attachment 281342 Patch 1202517 6 281342 achristensen 2016-06-15 00:28:17 -0700 Comment on attachment 281342 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281342&action=review > Tools/Scripts/build-webkit:247 > + removeCMakeCache(@featureArgs); Strange- this isn't called anywhere else. What is gtk doing? > Tools/Scripts/webkitdirs.pm:1908 > + my $winConfiguration = File::Spec->catdir(sourceDir(), "Source", "cmake", "OptionsWin.cmake"); Yep, cmakeBasedPortName() would return WinCairo or AppleWin. 1202518 7 pvollan 2016-06-15 00:31:38 -0700 (In reply to comment #6) > Comment on attachment 281342 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281342&action=review > > > Tools/Scripts/build-webkit:247 > > + removeCMakeCache(@featureArgs); > > Strange- this isn't called anywhere else. What is gtk doing? > It is also called on other platforms. Maybe we should avoid duplicating this call? Thanks for reviewing! 1202519 8 pvollan 2016-06-15 00:31:57 -0700 (In reply to comment #4) > I think you need to check the variable is already defined before setting. > > > if (DEFINED ${_name}) > > Because the variable can be given like cmake -D ENABLE_MATHML=OFF if I > invoke build-webkit --no-mathml. > > This change affects developers who don't use build-webkit for incremental > builds like me. > In incremental builds, cmake will be run automatically by invoking ninja or > Visual Studio's build command if CMake scripts have been modified. > In this time, build options will be restored from CMakeCache.txt. Thanks for looking into this :) The latest patch deletes the CMake cache file if options have been modified. Will this work for you? 1202523 9 fujii 2016-06-15 00:57:30 -0700 (In reply to comment #8) > The latest patch deletes the CMake cache file if options have been modified. > Will this work for you? Yes. Thank you. 1202592 10 281342 achristensen 2016-06-15 09:45:59 -0700 Comment on attachment 281342 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281342&action=review >>> Tools/Scripts/build-webkit:247 >>> + removeCMakeCache(@featureArgs); >> >> Strange- this isn't called anywhere else. What is gtk doing? > > It is also called on other platforms. Maybe we should avoid duplicating this call? > > Thanks for reviewing! Oh, I gripped before applying this patch and found one call. I thought I had applied this patch. You're right. 1202599 11 281342 commit-queue 2016-06-15 10:06:30 -0700 Comment on attachment 281342 Patch Clearing flags on attachment: 281342 Committed r202095: <http://trac.webkit.org/changeset/202095> 1202600 12 commit-queue 2016-06-15 10:06:34 -0700 All reviewed patches have been landed. Closing bug. 281261 2016-06-14 08:36:35 -0700 2016-06-15 00:22:23 -0700 Patch bug-158727-20160614083811.patch text/plain 1494 pvollan SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDIwMjA0 MykKKysrIENoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE2LTA2 LTE0ICBQZXIgQXJuZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbV2lu XVtDTWFrZV0gQ2hhbmdlcyBpbiBXZWJLaXQgb3B0aW9ucyBhcmUgbm90IHJlZmxlY3RlZCBpbiBp bmNyZW1lbnRhbCBidWlsZHMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xNTg3MjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBXaGVuIHRoZSBDTWFrZSAnb3B0aW9uJyBjb21tYW5kIGlzIHVzZWQsIGl0IGlz IG9ubHkgcG9zc2libGUgdG8gcHJvdmlkZSBhbiBpbml0aWFsIHZhbHVlLgorICAgICAgICBJbmNy ZW1lbnRhbCBidWlsZHMgd2lsbCB1c2UgdGhlIGNhY2hlZCB2YWx1ZS4gV2UgY2FuIHVzZSB0aGUg J3NldCcgY29tbWFuZCBpbnN0ZWFkLCB3aGljaAorICAgICAgICB3aWxsIGNyZWF0ZSBhIENNYWtl IHZhcmlhYmxlIHdoaWNoIGlzIG5vdCBwZXJzaXN0YW50LgorCisgICAgICAgICogU291cmNlL2Nt YWtlL1dlYktpdEZlYXR1cmVzLmNtYWtlOgorCiAyMDE2LTA2LTEzICBSb21haW4gQmVsbGVzc29y dCAgPHJvbWFpbi5iZWxsZXNzb3J0QGNyZi5jYW5vbi5mcj4KIAogICAgICAgICBbR1RLXSBFbmFi bGluZyBTaGFkb3cgRE9NIGJ5IGRlZmF1bHQKSW5kZXg6IFNvdXJjZS9jbWFrZS9XZWJLaXRGZWF0 dXJlcy5jbWFrZQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvY21ha2UvV2ViS2l0RmVhdHVyZXMuY21h a2UJKHJldmlzaW9uIDIwMTk4NikKKysrIFNvdXJjZS9jbWFrZS9XZWJLaXRGZWF0dXJlcy5jbWFr ZQkod29ya2luZyBjb3B5KQpAQCAtMjcwLDcgKzI3MCw3IEBAIG1hY3JvKFdFQktJVF9PUFRJT05f RU5EKQogICAgICAgICAgICAgc2V0KF9NQVhfRkVBVFVSRV9MRU5HVEggJHtfbmFtZV9sZW5ndGh9 KQogICAgICAgICBlbmRpZiAoKQogCi0gICAgICAgIG9wdGlvbigke19uYW1lfSAiJHtfV0VCS0lU X0FWQUlMQUJMRV9PUFRJT05TX0RFU0NSSVBUSU9OXyR7X25hbWV9fSIgJHtfV0VCS0lUX0FWQUlM QUJMRV9PUFRJT05TX0lOSVRJQUxfVkFMVUVfJHtfbmFtZX19KQorICAgICAgICBzZXQoJHtfbmFt ZX0gJHtfV0VCS0lUX0FWQUlMQUJMRV9PUFRJT05TX0lOSVRJQUxfVkFMVUVfJHtfbmFtZX19KQog ICAgICAgICBpZiAoTk9UIF9XRUJLSVRfQVZBSUxBQkxFX09QVElPTlNfSVNfUFVCTElDXyR7X25h bWV9KQogICAgICAgICAgICAgbWFya19hc19hZHZhbmNlZChGT1JDRSAke19uYW1lfSkKICAgICAg ICAgZW5kaWYgKCkK 281342 2016-06-15 00:22:25 -0700 2016-06-15 10:06:30 -0700 Patch bug-158727-20160615002401.patch text/plain 1881 pvollan SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIwMjA4NSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDE2LTA2LTE1ICBQZXIgQXJuZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNv bT4KKworICAgICAgICBbV2luXVtDTWFrZV0gQ2hhbmdlcyBpbiBXZWJLaXQgb3B0aW9ucyBhcmUg bm90IHJlZmxlY3RlZCBpbiBpbmNyZW1lbnRhbCBidWlsZHMuCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTg3MjcKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBEZWxldGUgQ01ha2UgY2FjaGUgZmlsZSBpZiBX ZWJLaXQgb3B0aW9ucyBoYXZlIGJlZW4gbW9kaWZpZWQuCisKKyAgICAgICAgKiBTY3JpcHRzL2J1 aWxkLXdlYmtpdDoKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdGRpcnMucG06CisgICAgICAgIChz aG91bGRSZW1vdmVDTWFrZUNhY2hlKToKKwogMjAxNi0wNi0xNCAgS2VpdGggTWlsbGVyICA8a2Vp dGhfbWlsbGVyQGFwcGxlLmNvbT4KIAogICAgICAgICBKU0JlbmNoIHNob3VsZCB1c2UgZ2VvbWV0 cmljIG1lYW4KSW5kZXg6IFRvb2xzL1NjcmlwdHMvYnVpbGQtd2Via2l0Cj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFRvb2xzL1NjcmlwdHMvYnVpbGQtd2Via2l0CShyZXZpc2lvbiAyMDIwODUpCisrKyBUb29scy9T Y3JpcHRzL2J1aWxkLXdlYmtpdAkod29ya2luZyBjb3B5KQpAQCAtMjQzLDYgKzI0Myw5IEBAIGlm IChpc0NNYWtlQnVpbGQoKSAmJiAhaXNBbnlXaW5kb3dzKCkpIHsKIAogbXkgJGJhc2VQcm9kdWN0 RGlyID0gYmFzZVByb2R1Y3REaXIoKTsKIGlmIChpc0FwcGxlV2luV2ViS2l0KCkgfHwgaXNXaW5D YWlybygpKSB7CisgICAgbXkgQGZlYXR1cmVBcmdzID0gY01ha2VBcmdzRnJvbUZlYXR1cmVzKCk7 CisgICAgcmVtb3ZlQ01ha2VDYWNoZShAZmVhdHVyZUFyZ3MpOworCiAgICAgY2hkaXJXZWJLaXQo KTsKICAgICBpZiAoZXhpdFN0YXR1cyhnZW5lcmF0ZUJ1aWxkU3lzdGVtRnJvbUNNYWtlUHJvamVj dCgpKSkgewogICAgICAgICBkaWUgIlJ1biBcIkM6L1Byb2dyYW0gRmlsZXMgKHg4NikvTWljcm9z b2Z0IFZpc3VhbCBTdHVkaW8gMTQuMC9WQy92Y3ZhcnNhbGwuYmF0XCIgYmVmb3JlIGJ1aWxkLXdl YmtpdCB3aGVuIHVzaW5nIG5pbmphIjsKSW5kZXg6IFRvb2xzL1NjcmlwdHMvd2Via2l0ZGlycy5w bQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtpdGRpcnMucG0JKHJldmlzaW9uIDIw MjA4NSkKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0ZGlycy5wbQkod29ya2luZyBjb3B5KQpAQCAt MTkwNCw2ICsxOTA0LDEzIEBAIHN1YiBzaG91bGRSZW1vdmVDTWFrZUNhY2hlKEApCiAgICAgICAg IHJldHVybiAxOwogICAgIH0KIAorICAgIGlmKGlzQW55V2luZG93cygpKSB7CisgICAgICAgIG15 ICR3aW5Db25maWd1cmF0aW9uID0gRmlsZTo6U3BlYy0+Y2F0ZGlyKHNvdXJjZURpcigpLCAiU291 cmNlIiwgImNtYWtlIiwgIk9wdGlvbnNXaW4uY21ha2UiKTsKKyAgICAgICAgaWYgKCRjYWNoZUZp bGVNb2RpZmllZFRpbWUgPCBzdGF0KCR3aW5Db25maWd1cmF0aW9uKS0+bXRpbWUpIHsKKyAgICAg ICAgICAgIHJldHVybiAxOworICAgICAgICB9CisgICAgfQorCiAgICAgcmV0dXJuIDA7CiB9CiAK