Bug 158727 – [Win][CMake] Changes in WebKit options are not reflected in incremental builds.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 281261 [details]
Patch

[Per Arne Vollan]

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.

[Alex Christensen]

I think GTK and EFL are using shouldRemoveCMakeCache is webkitdirs.pm.  Are we using that on Windows?

[Fujii Hironori]

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.

[Per Arne Vollan]

Created attachment 281342 [details]
Patch

[Alex Christensen]

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.

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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?

[Fujii Hironori]

(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.

[Alex Christensen]

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.

[WebKit Commit Bot]

Comment on attachment 281342 [details]
Patch

Clearing flags on attachment: 281342

Committed r202095: <http://trac.webkit.org/changeset/202095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.