Summary: | [Win][CMake] Changes in WebKit options are not reflected in incremental builds. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||
Component: | Tools / Tests | Assignee: | Per Arne Vollan <pvollan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, dbates, Hironori.Fujii, lforschler | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Per Arne Vollan
2016-06-14 00:25:22 PDT
Created attachment 281261 [details]
Patch
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. I think GTK and EFL are using shouldRemoveCMakeCache is webkitdirs.pm. Are we using that on Windows? 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.
Created attachment 281342 [details]
Patch
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? > Tools/Scripts/webkitdirs.pm:1908 > + my $winConfiguration = File::Spec->catdir(sourceDir(), "Source", "cmake", "OptionsWin.cmake"); Yep, cmakeBasedPortName() would return WinCairo or AppleWin. (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! (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? (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. 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! Oh, I gripped before applying this patch and found one call. I thought I had applied this patch. You're right. Comment on attachment 281342 [details] Patch Clearing flags on attachment: 281342 Committed r202095: <http://trac.webkit.org/changeset/202095> All reviewed patches have been landed. Closing bug. |