Bug 103162 – [cmake] Unify the way default features are enabled/disable on CMake based ports.

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Hugo Parente Lima]

After this patch you need to modify just one file to change the default value of a WebKit feature, the value can be overwritten by Platform.h, but this is another story.

The patch was tested just with EFL port and works nice, if I get the thumbs up I can do the changes for other ports or create the patch in a way that it doesn't touch BlackBerry and WinCE ports.

[Hugo Parente Lima]

Created attachment 175837 [details]
Patch

[Hugo Parente Lima]

Ooops, I forgot to remove cmakeconfig.h.cmake file, it auto generated by the patch and isn't needed.

[Laszlo Gombos]

Another common case is that a new ENABLE_X define is introduced to the project. How is that impacted (in terms of the number of files that needs to be touched) ?

[Hugo Parente Lima]

(In reply to comment #3)
> Another common case is that a new ENABLE_X define is introduced to the project. How is that impacted (in terms of the number of files that needs to be touched) ?

You just add it to FeatureDefaults.txt.

So later, if a port want to change it to a value different from the one defined on FeatureDefaults.txt it change the FeatureDefaultsPORT.txt.

Nowadays it need to change 3 files, FeatureList.pm, cmakeconfig.h and OptionsPORT.cmake

[Hugo Parente Lima]

Created attachment 176028 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 176028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176028&action=review

I think we should get rid of WebKitFeatures once for all and this refactoring sounds like a good opportunity for that. This has been an annoying "feature" for CMake. Currently it is really hard (if not impossible) to see what is enabled and what is not for any CMake port. By eliminating WebKitFeatures, we can have all the features disabled by default and on the platform specific file we see only what the port is enabling.

> Tools/Scripts/webkitperl/FeatureDefaultsBlackBerry.txt:41
> +ENABLE_VIEWPORT_REFLOW             = 1
> +ENABLE_VIEWPORT_REFLOW             = 1

Dup!

[Hugo Parente Lima]

(In reply to comment #6)
> (From update of attachment 176028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176028&action=review
> 
> I think we should get rid of WebKitFeatures once for all and this refactoring sounds like a good opportunity for that. This has been an annoying "feature" for CMake. Currently it is really hard (if not impossible) to see what is enabled and what is not for any CMake port. By eliminating WebKitFeatures, we can have all the features disabled by default and on the platform specific file we see only what the port is enabling.

I'm all for it, just waiting thumbs up from CMake port reviewers to push this patch forward.

> > Tools/Scripts/webkitperl/FeatureDefaultsBlackBerry.txt:41
> > +ENABLE_VIEWPORT_REFLOW             = 1
> > +ENABLE_VIEWPORT_REFLOW             = 1
> 
> Dup!

[Laszlo Gombos]

Comment on attachment 176028 [details]
Patch

Thanks for your patch. This is hard problem to tackle that requires by in from many. Overall I think the patch contains same great ideas, but I expect a few more rounds of reviews.

r- for making the Source directory dependent on the Tools directory.

This patch moves the defaults from the Source directory into the Tools directory. One of the project goals is that the Source directory should not be dependent on Tools and the Source directory should produce a functional WebKit library. It seems to me that this patch violates this goal. I think the we should keep the defaults under the Source directory. I am not sure what is the best directory under Source to store the defaults as that depends on which part of the build system would actually read the defaults. Source/cmake is still probably the safest bet.

I think we should explore the possibility of considering a different file format for "FeatureDefaults" to enable the file not only processed by scripts but also to be a proper .h file that could be included in the project source directly. So instead of

"ENABLE_XXX = 1"

the file would esentially do the following (like in Platform.h)

#if !defined(ENABLE_XXX)
#define ENABLE_XXX 1
#endif

As a first step it make sense to only do this change for CMake based ports, but I think we should choose a file format that enables the possibility for other ports to use it in C/C++ code directly.

[Hugo Parente Lima]

Hi,

(In reply to comment #8)
> (From update of attachment 176028 [details])
> Thanks for your patch. This is hard problem to tackle that requires by in from many. Overall I think the patch contains same great ideas, but I expect a few more rounds of reviews.
> 
> r- for making the Source directory dependent on the Tools directory.
> 
> This patch moves the defaults from the Source directory into the Tools directory. One of the project goals is that the Source directory should not be dependent on Tools and the Source directory should produce a functional WebKit library. It seems to me that this patch violates this goal. I think the we should keep the defaults under the Source directory. I am not sure what is the best directory under Source to store the defaults as that depends on which part of the build system would actually read the defaults. Source/cmake is still probably the safest bet.

I agree.
 
> I think we should explore the possibility of considering a different file format for "FeatureDefaults" to enable the file not only processed by scripts but also to be a proper .h file that could be included in the project source directly. So instead of
> 
> "ENABLE_XXX = 1"
> 
> the file would esentially do the following (like in Platform.h)
> 
> #if !defined(ENABLE_XXX)
> #define ENABLE_XXX 1
> #endif

This means killing cmakeconfig.h ? and adding a lot of -DENABLE_XX to the compiler command line, I'm still not sure if this is good, but at least reduce the build system influence in the compilation process, i.e. we don't have the build system generating source files for us (cmakeconfig.h)
 
> As a first step it make sense to only do this change for CMake based ports, but I think we should choose a file format that enables the possibility for other ports to use it in C/C++ code directly.

We could do:

"Source/featuresconfig.h" with a bunch of

#if !defined(ENABLE_XXX)
#define ENABLE_XXX 1
#endif

because we need a place where to find all WebKit existing features to enable cmake and build-webkit script to figure out what it can enable/disable and add the -DENABLE... on compiler command line, then this file can include the port specific features header at top.

#if PLATFORM(FOO_BAR)
#include "foobarconfig.h"
#endif

What do you think?

[Laszlo Gombos]

(In reply to comment #9)

> > So instead of
> > 
> > "ENABLE_XXX = 1"
> > 
> > the file would esentially do the following (like in Platform.h)
> > 
> > #if !defined(ENABLE_XXX)
> > #define ENABLE_XXX 1
> > #endif
> 
> This means killing cmakeconfig.h ? and adding a lot of -DENABLE_XX to the compiler command line.

As discussed on irc, not necessary.

> We could do:
> 
> "Source/featuresconfig.h" with a bunch of
> 
> #if !defined(ENABLE_XXX)
> #define ENABLE_XXX 1
> #endif

Maybe we should put it under cmake directory for now and move it later if it gets used outside of cmake based ports. In the ChangeLog and perhaps in the file itself as a comment we should mention that the intention is that this file can be processed by a script or directly by a C compiler to enable wider use later, so this will be a very small subset of what is normally legal in a .h file. 

I think it is important to keep featuresconfig.h and featuresconfig-portX.h very simple and they should not be including each other.

[Hugo Parente Lima]

I took a look on what we discussed last week, there's just a problem with the .h approach, GCC throw warning when some constant is redefined, so even if we use the .h file format, those .h files can't be used directly on the source code to avoid lots of warnings about constants being redefined :-/.

[Hugo Parente Lima]

Talking with Laszlo again, it's possible (and simple) to avoid the constant redefinition, I'll upload a new patch soon.

[Hugo Parente Lima]

Created attachment 179973 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 179973 [details]
Patch

Isn't time to get rid of this global Features.h once for all? I honestly don't see any benefit of having it and I would love to see all the features we are enabling for EFL in a single place. It would also be a enabler for bug 85456 on CMake-based ports.

[Hugo Parente Lima]

Yep, it can be removed and just the port specific still there, the good part is that things like "build-webkit --help --port" will only show options that make sense for the port.

Laszlo, what do you think?

[Laszlo Gombos]

(In reply to comment #14)
> (From update of attachment 179973 [details])
> Isn't time to get rid of this global Features.h once for all? I honestly don't see any benefit of having it

One reason to have a global feature list is to follow DRY (do not repeat yourself) principles. Why should have have 3 copies of a feature default if the default is the same for all 3 CMake based ports ? 

>  and I would love to see all the features we are enabling for EFL in a single place. 

I think that single place at the moment is CMakeCache.txt -e.g.  grep "ENABLE_.*=ON" CMakeCache.txt

[Laszlo Gombos]

Comment on attachment 179973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179973&action=review

I think the direction looks good and the patch is getting better every the time, but we still need a few more rounds of reviews.

One of the goal of this work was to remove the cmakeconfig.h.cmake file form the source tree, but it seems to me that the patch does not get rid of the file. Are you using svn or git ? Have you tried "svn rm" ?

> Source/cmake/Features.h:3
> +#ifndef Features_h
> +#define Features_h
> +

I think we should try to do a regular C include here to include the port specific files before you default to the port independent generic defaults and use the C pre-processor to compute the defaults for the defines. See some more comments on this below..

#include <wtf/Platform.h>

#if PLATFORM(BLACKBERRY)
#include "FeaturesBlackBerry.h"
#endif

I think it is misleading if we call these files header files but they are not evaluated as header files - this is why I think we should use the C pre-processor to evaluate these header files. 

An other approach - which I favor less - would be to call these files a .in file and do not treat them as header files any more.

> Source/cmake/FeaturesBlackBerry.h:6
> +#define ENABLE_ANIMATION_API 1
> +#define ENABLE_BATTERY_STATUS 1
> +#define ENABLE_BLOB 1

I think the port specific files need to also follow the same pattern as the generic Features.h for consistency.

> Source/cmake/FeaturesEfl.h:6
> +#define ENABLE_3D_RENDERING 0
> +#define ENABLE_ACCELERATED_2D_CANVAS 0
> +#define ENABLE_ANIMATION_API 1

Ditto.

> Source/cmake/FeaturesWinCE.h:6
> +#define ENABLE_DRAG_SUPPORT 0
> +#define ENABLE_FTPDIR       0
> +#define ENABLE_INSPECTOR    0

Ditto.

> Tools/Scripts/webkitperl/FeatureList.pm:459
> +    # Reset WebKit @features array and populate it with Source/cmake/FeatureDefaults.config file

FeatureDefaults.config file ? Where is this file coming from ?

> Tools/Scripts/webkitperl/FeatureList.pm:494
> +    # Fill the default values using Port specific FeatureList
> +    my $featureFile = "${baseDir}Features${cmakePortName}.h";
> +
> +    if (open(FEATURES, $featureFile) or die("Failed to open $featureFile.\n")) {

What do you think about using the C pre-processor to compute the port specific defaults ? 

Try running "${CMAKE_CXX_COMPILER} -E -dM -IWTF;. -DWTF_PLATFORM_XXX "Features.h" and see what is the best way to process the output to populate featureIndex.

[Hugo Parente Lima]

(In reply to comment #17)
> (From update of attachment 179973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179973&action=review
> 
> I think the direction looks good and the patch is getting better every the time, but we still need a few more rounds of reviews.
> 
> One of the goal of this work was to remove the cmakeconfig.h.cmake file form the source tree, but it seems to me that the patch does not get rid of the file. Are you using svn or git ? Have you tried "svn rm" ?

I'm using git, but I forgot to remove the cmakeconfig.h.cmake this time, sorry :-).
 
> > Source/cmake/Features.h:3
> > +#ifndef Features_h
> > +#define Features_h
> > +
> 
> I think we should try to do a regular C include here to include the port specific files before you default to the port independent generic defaults and use the C pre-processor to compute the defaults for the defines. See some more comments on this below..
> 
> #include <wtf/Platform.h>
> 
> #if PLATFORM(BLACKBERRY)
> #include "FeaturesBlackBerry.h"
> #endif
> 
> I think it is misleading if we call these files header files but they are not evaluated as header files - this is why I think we should use the C pre-processor to evaluate these header files. 
> 
> An other approach - which I favor less - would be to call these files a .in file and do not treat them as header files any more.

While writing the patch I noticed that no matter the file format we always will need to create a header file (cmakeconfig.h) with all features enabled/disabled by the user, so including any of those files (FeatureXXX.h) directly is pointless, because all real information about what's enabled/disabled will reside in the generated file (cmakeconfig.h).
 
> > Source/cmake/FeaturesBlackBerry.h:6
> > +#define ENABLE_ANIMATION_API 1
> > +#define ENABLE_BATTERY_STATUS 1
> > +#define ENABLE_BLOB 1
> 
> I think the port specific files need to also follow the same pattern as the generic Features.h for consistency.
> 
> > Source/cmake/FeaturesEfl.h:6
> > +#define ENABLE_3D_RENDERING 0
> > +#define ENABLE_ACCELERATED_2D_CANVAS 0
> > +#define ENABLE_ANIMATION_API 1
> 
> Ditto.
> 
> > Source/cmake/FeaturesWinCE.h:6
> > +#define ENABLE_DRAG_SUPPORT 0
> > +#define ENABLE_FTPDIR       0
> > +#define ENABLE_INSPECTOR    0
> 
> Ditto.
> 
> > Tools/Scripts/webkitperl/FeatureList.pm:459
> > +    # Reset WebKit @features array and populate it with Source/cmake/FeatureDefaults.config file
> 
> FeatureDefaults.config file ? Where is this file coming from ?

A typo, this was just a candidate name.
 
> > Tools/Scripts/webkitperl/FeatureList.pm:494
> > +    # Fill the default values using Port specific FeatureList
> > +    my $featureFile = "${baseDir}Features${cmakePortName}.h";
> > +
> > +    if (open(FEATURES, $featureFile) or die("Failed to open $featureFile.\n")) {
> 
> What do you think about using the C pre-processor to compute the port specific defaults ? 
> 
> Try running "${CMAKE_CXX_COMPILER} -E -dM -IWTF;. -DWTF_PLATFORM_XXX "Features.h" and see what is the best way to process the output to populate featureIndex.

But what about WinCE port? They use cmake too and probably not g++.

[Hugo Parente Lima]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 179973 [details] [details])
> > Isn't time to get rid of this global Features.h once for all? I honestly don't see any benefit of having it
> 
> One reason to have a global feature list is to follow DRY (do not repeat yourself) principles. Why should have have 3 copies of a feature default if the default is the same for all 3 CMake based ports ? 

I like the idea of not sharing feature configuration files with other ports and share just the read/use mechanism instead. This would simplify the things a lot, e.g. the BlackBerry port has a lot of ENABLE flags used only on BlackBerry port, so listing this ENABLE flags on all other ports make not much sense because it's useless on all ports but the BlackBerry one.

I don't think it hurts too much the DRY principle, we are talking about configuration files with just few lines (less than ten lines) of text repeated, BTW we already have things similar to this on WebKit nowadays, just look at jhbuild config files, each port have their own files sharing nothing with each other because sharing those config files would cause more trouble than fun.
 
> >  and I would love to see all the features we are enabling for EFL in a single place. 
> 
> I think that single place at the moment is CMakeCache.txt -e.g.  grep "ENABLE_.*=ON" CMakeCache.txt

[Hugo Parente Lima]

Created attachment 181409 [details]
Patch

[EFL EWS Bot]

Comment on attachment 181409 [details]
Patch

Attachment 181409 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15742184

[Hugo Parente Lima]

EFL always add the sources:

        accessibility/atk/AXObjectCacheAtk.cpp
        accessibility/atk/WebKitAccessibleHyperlink.cpp
        accessibility/atk/WebKitAccessibleInterfaceAction.cpp
        accessibility/atk/WebKitAccessibleInterfaceComponent.cpp
        accessibility/atk/WebKitAccessibleInterfaceDocument.cpp
        accessibility/atk/WebKitAccessibleInterfaceEditableText.cpp
        accessibility/atk/WebKitAccessibleInterfaceHyperlinkImpl.cpp
        accessibility/atk/WebKitAccessibleInterfaceHypertext.cpp
        accessibility/atk/WebKitAccessibleInterfaceImage.cpp
        accessibility/atk/WebKitAccessibleInterfaceSelection.cpp
        accessibility/atk/WebKitAccessibleInterfaceTable.cpp
        accessibility/atk/WebKitAccessibleInterfaceText.cpp
        accessibility/atk/WebKitAccessibleInterfaceValue.cpp
        accessibility/atk/WebKitAccessibleUtil.cpp
        accessibility/atk/WebKitAccessibleWrapperAtk.cpp

on WebCore no matter if ATK was found or not, this is buggy because some of those files include atk.h header.

[Hugo Parente Lima]

Created attachment 181525 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 181525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181525&action=review

> Source/WebCore/PlatformEfl.cmake:388
>  if (ENABLE_ACCESSIBILITY)
> +    list(APPEND WebCore_SOURCES
> +        accessibility/atk/AccessibilityObjectAtk.cpp
> +        accessibility/atk/AXObjectCacheAtk.cpp
> +        accessibility/atk/WebKitAccessibleHyperlink.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceAction.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceComponent.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceDocument.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceEditableText.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceHyperlinkImpl.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceHypertext.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceImage.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceSelection.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceTable.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceText.cpp
> +        accessibility/atk/WebKitAccessibleInterfaceValue.cpp
> +        accessibility/atk/WebKitAccessibleUtil.cpp
> +        accessibility/atk/WebKitAccessibleWrapperAtk.cpp
> +    )
>      list(APPEND WebCore_INCLUDE_DIRECTORIES

Looks wrong to me. What I have seen so far is we add every file on the source list regardless if the feature is enabled or not and the file itself has it's source code guarded by features flags.

[Laszlo Gombos]

(In reply to comment #22)

[...]
>         accessibility/atk/WebKitAccessibleWrapperAtk.cpp
> 
> on WebCore no matter if ATK was found or not, this is buggy because some of those files include atk.h header.

I think this is an independent issue and should be discussed as a separate bug and a separate patch. 

The direction has been to include the conditionals in the .c/.cpp files where it is possible especially in code that is shared with other ports.

[Gyuyoung Kim]

(In reply to comment #25)
> (In reply to comment #22)
> 
> [...]
> >         accessibility/atk/WebKitAccessibleWrapperAtk.cpp
> > 
> > on WebCore no matter if ATK was found or not, this is buggy because some of those files include atk.h header.
> 
> I think this is an independent issue and should be discussed as a separate bug and a separate patch. 
> 
> The direction has been to include the conditionals in the .c/.cpp files where it is possible especially in code that is shared with other ports.

I file a bug 106290 for this issue.

[Hugo Parente Lima]

So I'll re-upload a new version of the patch after the fix for bug 106290 gets landed.

[Daniel Bates]

Comment on attachment 181525 [details]
Patch

Out of curiosity, did you consider the approach used by the Qt port in build-webkit [1], which selectively overrides the feature defaults based on the defines specified in the qmake project files?

[1] <http://trac.webkit.org/browser/trunk/Tools/Scripts/build-webkit?rev=137612#L69>

[Daniel Bates]

(In reply to comment #28)
> (From update of attachment 181525 [details])
> Out of curiosity, did you consider the approach used by the Qt port in build-webkit [1], which selectively overrides the feature defaults based on the defines specified in the qmake project files?

Disregard this question. I meant to ask: did you consider having FeatureX.config extend the set of feature defines for defines not specified in FeatureList.pm and otherwise selectively override the features defined in FeatureList.pm?

Currently the patch overrides all the feature defines in FeatureList.pm with the feature defines specified in FeatureX.config. Alternatively, we could make setting the default value for each feature define in FeatureList.pm the convention (*) and have FeatureX.config be an optional file for specifying either additional port-specific defines or overriding defines in FeatureList.pm. Although, I propose using FeatureX.config only for the former. That is, to specify additional port-specific defines.

(*) This is current convention in FeatureList.pm. That is, to enable a feature, say ENABLE_ANIMATION_API, by default on only the BlackBerry and EFL ports you specify default => isBlackBerry() || isEfl() in the hash reference initialization for this feature.

[Hugo Parente Lima]

(In reply to comment #29)
> Disregard this question. I meant to ask: did you consider having FeatureX.config extend the set of feature defines for defines not specified in FeatureList.pm and otherwise selectively override the features defined in FeatureList.pm?
> 
> Currently the patch overrides all the feature defines in FeatureList.pm with the feature defines specified in FeatureX.config. Alternatively, we could make setting the default value for each feature define in FeatureList.pm the convention (*) and have FeatureX.config be an optional file for specifying either additional port-specific defines or overriding defines in FeatureList.pm. Although, I propose using FeatureX.config only for the former. That is, to specify additional port-specific defines.

Hi,

One of the things I want to achieve with this patch is not showing things that makes no sense on some ports, e.g. Why should I be able to set or see in the help message a Blackberry specific option? Why should I be able to disable tiled backing store on Qt if doing this the port probably will not even compile or my choice will be overwritten by qmake re-enabling it? Those "options that aren't options" just polutes and confuses the output of build-webkit script. So my proposed solution is: Each and only each port known what is really an option and can safely  be enabled/disabled or not and those real options can go in just one place (the FeaturesX.config files).

The reason why I didn't touch the big array with (almost) all options on FeaturesList.pm is just a matter of not perturbing other ports, i.e. just patch aims to change just the CMake based ports.

 
> (*) This is current convention in FeatureList.pm. That is, to enable a feature, say ENABLE_ANIMATION_API, by default on only the BlackBerry and EFL ports you specify default => isBlackBerry() || isEfl() in the hash reference initialization for this feature.

Yes, but this feature can be overwritten by WebKitFeatures.cmake or OptionsX.cmake and if it isn't on cmakeconfig.h setting or unsetting it may have no results at all.

[Hugo Parente Lima]

Sorry for the typos, I meant:

"Why should I be able to set or see in the help message a Blackberry specific option when compiling the Efl port?"

and

"The reason why I didn't touch the big array with (almost) all options on FeaturesList.pm is just a matter of not perturbing other ports, i.e. the patch aims to change just the CMake based ports."

[Hugo Parente Lima]

Created attachment 181932 [details]
Patch

[Laszlo Gombos]

Comment on attachment 181932 [details]
Patch

It seems to me that this patch will introduce some regressions, r- for that.

> > Try running "${CMAKE_CXX_COMPILER} -E -dM -IWTF;. -DWTF_PLATFORM_XXX "Features.h" and see what is the best way to process the output to populate featureIndex.
> 
> But what about WinCE port? They use cmake too and probably not g++.

CMAKE_CXX_COMPILER is set to the MS Compiler by default on Windows which also has a way to invoke the preprocessor - http://msdn.microsoft.com/en-us/library/ms924239.aspx .

> "Why should I be able to set or see in the help message a Blackberry specific option when compiling the Efl port?"

This is an ambitious goal. 

If this is the intention than I expect that you would need to detect all the dependencies that are responsible for optional build options (e.g you should only list ACCESSIBILITY is a webkit-build option for EFL if ATK is available in the system) every time build-webkit --help runs (and repeat the detection when the actual build is executed).

It might make sense to implement this functionality as a separate patch as I do not think this is essential for what you're trying to do for this bug. It also seems to me that your patch introduces a few regressions:

1./ build-webkit --efl defaults to ACCESSIBILITY OFF even though I have ATK in the system. This seems to be a regression and it should default to ON when ATK is available.

2./ build-webkit --wince no longer list SVG as a build option. I suspect (I have not tried) that this patch will also make SVG turned OFF by default for WinCE. This seems to be a regression as previously SVG was turned on by default on all CMake based ports in WebKitFeatures.cmake.


The description of this bug is the following:

> After this patch you need to modify just one file to change the default value of a WebKit feature.

It seems to me that after this patch if a feature is meant to be supported or disabled for all 3 CMake based ports, it would need to be added to 3 files (instead of 1 file as it was promised or instead of 2 files which is the current practice). Can you perhaps restate the goal of this bug or maybe go back to the original idea of sharing the defaults between CMake based ports ? 

> Source/cmake/OptionsBlackBerry.cmake:24
> -    add_definitions(-DWTF_USE_EXPORT_MACROS=1)
> +    SET_HARDCODED_DEFINE(WTF_USE_EXPORT_MACROS 1)

It seems to me that this will turn definitions that are passed to the compiler on the command line into CMake variables that are listed in CMakeCache.txt and will now be configuration option in tools like cmake-gui. Why ? Also why do you need to add this information to cmakeconfig.h.in ?

[Hugo Parente Lima]

(In reply to comment #33)
> (From update of attachment 181932 [details])
> It seems to me that this patch will introduce some regressions, r- for that.
> 
> > > Try running "${CMAKE_CXX_COMPILER} -E -dM -IWTF;. -DWTF_PLATFORM_XXX "Features.h" and see what is the best way to process the output to populate featureIndex.
> > 
> > But what about WinCE port? They use cmake too and probably not g++.
> 
> CMAKE_CXX_COMPILER is set to the MS Compiler by default on Windows which also has a way to invoke the preprocessor - http://msdn.microsoft.com/en-us/library/ms924239.aspx .
> 
> > "Why should I be able to set or see in the help message a Blackberry specific option when compiling the Efl port?"
> 
> This is an ambitious goal. 
> 
> If this is the intention than I expect that you would need to detect all the dependencies that are responsible for optional build options (e.g you should only list ACCESSIBILITY is a webkit-build option for EFL if ATK is available in the system) every time build-webkit --help runs (and repeat the detection when the actual build is executed).

Hi,

No, you misinterpreted the sentence:

"showing things that makes no sense on some ports"

I meant show options that's possible with the e.g. Efl port source code, not what's possible with the Efl port on your machine, this explains why build-webkit script doesn't (and shouldn't) do any check, if you enable a feature X that needs libY and you don't have libY you will be notified later, but you will know that Efl source code supports the feature X because it was shown on build-webkit script.

IMO enable a feature secretly only if lib X is found isn't a good idea, "explicit is better than implicit" fits here, the user should know what features he want.

> It might make sense to implement this functionality as a separate patch as I do not think this is essential for what you're trying to do for this bug. It also seems to me that your patch introduces a few regressions:
> 
> 1./ build-webkit --efl defaults to ACCESSIBILITY OFF even though I have ATK in the system. This seems to be a regression and it should default to ON when ATK is available.
> 
> 2./ build-webkit --wince no longer list SVG as a build option. I suspect (I have not tried) that this patch will also make SVG turned OFF by default for WinCE. This seems to be a regression as previously SVG was turned on by default on all CMake based ports in WebKitFeatures.cmake.

The contents of FeaturesXXX.config was meant to be reviewed by each port reviewer, because I'm not able for example to build the blackberry or WinCE port on my machine, I'll do the adjustments.
 
> 
> The description of this bug is the following:
> 
> > After this patch you need to modify just one file to change the default value of a WebKit feature.
> 
> It seems to me that after this patch if a feature is meant to be supported or disabled for all 3 CMake based ports, it would need to be added to 3 files (instead of 1 file as it was promised or instead of 2 files which is the current practice). Can you perhaps restate the goal of this bug or maybe go back to the original idea of sharing the defaults between CMake based ports ? 

Suppose you create a new WebKit feature, isn't polite to enable it on all ports, so you wont enable it on all ports, you just announce it on webkit-dev and let the port "onwers" to add the flag if they want, so... just one file :-).
 
> > Source/cmake/OptionsBlackBerry.cmake:24
> > -    add_definitions(-DWTF_USE_EXPORT_MACROS=1)
> > +    SET_HARDCODED_DEFINE(WTF_USE_EXPORT_MACROS 1)
> 
> It seems to me that this will turn definitions that are passed to the compiler on the command line into CMake variables that are listed in CMakeCache.txt and will now be configuration option in tools like cmake-gui. Why ? Also why do you need to add this information to cmakeconfig.h.in ?

IIRC those variables will not end on cmakecache because I'm not using "CACHE" flag on the set command, so they wont be listed on cmakecache, however I need to test it. About the other question, It's possible to do similar questions with the current state of CMake build files on trunk: Why do you need cmakeconfig.h.in if you can pass all variable on compiler command line? or... Why nowadays some variable are defined on cmakeconfig.h and some on compiler command line?

I put them on cmakeconfig.h to be easier to find their values, much better than run cmake on verbose mode and search them in a huge command line, some of them could fit on Platform.h, but IMO is better to keep the WebKit source tree free from port specific things as much as possible, so cmakeconfig.h.

[Laszlo Gombos]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 181932 [details] [details])

> No, you misinterpreted the sentence:
> 
> "showing things that makes no sense on some ports"
> 
> I meant show options that's possible with the e.g. Efl port source code, not what's possible with the Efl port on your machine

I see. Please make sure that you capture key goals like this clearly in the Changelog.

I still think that this idea should be discussion as a separate patch.

> Suppose you create a new WebKit feature, isn't polite to enable it on all ports, so you wont enable it on all ports, you just announce it on webkit-dev and let the port "onwers" to add the flag if they want, so... just one file :-).

This is a good idea (I have been advocating this myself as well). However without a central place for all the available feature flags this will make it harder for a port to discover what features are not yet enabled for a specific port - which I though was one of the motivation of this work. 

If/when this lands this will difference from other ports/build systems needs to be pointed out in http://trac.webkit.org/wiki/AddingFeatures .

[Hugo Parente Lima]

Created attachment 185861 [details]
Patch

Patch updated but only config for Efl is here. So this is mostly for yet another review round by Efl maintainers.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 185861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185861&action=review

Better late than never to chime in, I guess :-)

I like the idea of reducing the number of files that need to be changed, and autogenerating cmakeconfig.h is very ingenious.

So my concerns so far are more about the implementation than the aim itself. The points I make below should be read more as questions than assertions, as it's likely that I'm missing something after so many comments and changes.

The idea that we sort of disregard the definitions in FeaturesList.pm in favor of what we define in the hand-crafted FeatureFoo.config files doesn't sound that good to me: I agree with dydx's points in comment #29; if the biggest problem is with the "options that are not real options for a given port", I think it would make sense to improve our webkitperl infrastructure not to show those.

This also makes me think of two distinct groups we need to take into consideration: WebKit developers, who use build-webkit and want to have as many features as possible enabled, and downstream (end users, packagers etc), who prefer only stable features enabled by default. If we generate defaults for both groups from the same file, I'm afraid we'll end up with either developers (and bots) building ports with fewer features enabled (and thus reducing our coverage) or downstreams having to find out for themselves that besides the usual CMake stuff we also happen to have some .config files hidden somewhere which indicates which features are enabled and disabled by default.

The amount of manual parsing we end up doing for those FeatureFoo.config files also seems to indicate we can simplify things. Related point: right now, if one adds a new feature to WebKit, it is necessary to edit cmakeconfig.h.cmake and WebKitFeatures.cmake; if we drop WebKitFeatures.cmake and use Feature<Port>.config instead, does it mean one will have to edit N * <CMake-based ports> files?

> Source/cmake/OptionsEfl.cmake:97
> -    set(ENABLE_PLUGIN_PROCESS 1)
> +    set(ENABLE_PLUGIN_PROCESS ON)

This kind of change, while good in terms of consistency, is not really needed for this patch, so I'd rather to that separately and keep this one as small as possible.

> Source/cmake/WebKitFeatures.cmake:56
> +    set(CMK_CONFIG_H_IN "${CMAKE_BINARY_DIR}/cmakeconfig.h.in")

I'd rather follow our general habit of not abbreviating things and use a name such as "CMAKE_CONFIG_H_IN" here.

> Source/cmake/WebKitFeatures.cmake:73
> +    file(MD5 ${CMK_CONFIG_H_IN} CMK_CONFIG_H_IN_NEW_MD5)
> +    if (NOT "${CMK_CONFIG_H_IN_MD5}" STREQUAL "${CMK_CONFIG_H_IN_NEW_MD5}")
> +        message(STATUS "Creating cmakeconfig.h")
> +        configure_file(${CMK_CONFIG_H_IN} "${CMAKE_BINARY_DIR}/cmakeconfig.h" @ONLY)
> +        set(CMK_CONFIG_H_IN_MD5 "${CMK_CONFIG_H_IN_NEW_MD5}" CACHE STRING "Md5 of ${CMK_CONFIG_H_IN}" FORCE)
> +    endif ()

Is this really needed? I mean, does regenerating this file cause unnecessary recompilations of other files?

[Hugo Parente Lima]

(In reply to comment #37)
> (From update of attachment 185861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185861&action=review
> 
> Better late than never to chime in, I guess :-)
> 
> I like the idea of reducing the number of files that need to be changed, and autogenerating cmakeconfig.h is very ingenious.
> 
> So my concerns so far are more about the implementation than the aim itself. The points I make below should be read more as questions than assertions, as it's likely that I'm missing something after so many comments and changes.
> 
> The idea that we sort of disregard the definitions in FeaturesList.pm in favor of what we define in the hand-crafted FeatureFoo.config files doesn't sound that good to me: I agree with dydx's points in comment #29; if the biggest problem is with the "options that are not real options for a given port", I think it would make sense to improve our webkitperl infrastructure not to show those.
> 
> This also makes me think of two distinct groups we need to take into consideration: WebKit developers, who use build-webkit and want to have as many features as possible enabled, and downstream (end users, packagers etc), who prefer only stable features enabled by default. If we generate defaults for both groups from the same file, I'm afraid we'll end up with either developers (and bots) building ports with fewer features enabled (and thus reducing our coverage) or downstreams having to find out for themselves that besides the usual CMake stuff we also happen to have some .config files hidden somewhere which indicates which features are enabled and disabled by default.
> 
> The amount of manual parsing we end up doing for those FeatureFoo.config files also seems to indicate we can simplify things. Related point: right now, if one adds a new feature to WebKit, it is necessary to edit cmakeconfig.h.cmake and WebKitFeatures.cmake; if we drop WebKitFeatures.cmake and use Feature<Port>.config instead, does it mean one will have to edit N * <CMake-based ports> files?

Not really, if you add a feature you will notify webkit-dev about it and someone will edit just one Feature<Port>.config file adding the flag if such flag is useful for the port. I'm open for new ideas and for sure there are better ways to do this, however I still believe that this patch improves the current situation.

> 
> > Source/cmake/OptionsEfl.cmake:97
> > -    set(ENABLE_PLUGIN_PROCESS 1)
> > +    set(ENABLE_PLUGIN_PROCESS ON)
> 
> This kind of change, while good in terms of consistency, is not really needed for this patch, so I'd rather to that separately and keep this one as small as possible.

This is a cosmetic change, without this the feature summary will show "1" instead of "ON", so it somehow relates with this patch.
 
> > Source/cmake/WebKitFeatures.cmake:56
> > +    set(CMK_CONFIG_H_IN "${CMAKE_BINARY_DIR}/cmakeconfig.h.in")
> 
> I'd rather follow our general habit of not abbreviating things and use a name such as "CMAKE_CONFIG_H_IN" here.
> 
> > Source/cmake/WebKitFeatures.cmake:73
> > +    file(MD5 ${CMK_CONFIG_H_IN} CMK_CONFIG_H_IN_NEW_MD5)
> > +    if (NOT "${CMK_CONFIG_H_IN_MD5}" STREQUAL "${CMK_CONFIG_H_IN_NEW_MD5}")
> > +        message(STATUS "Creating cmakeconfig.h")
> > +        configure_file(${CMK_CONFIG_H_IN} "${CMAKE_BINARY_DIR}/cmakeconfig.h" @ONLY)
> > +        set(CMK_CONFIG_H_IN_MD5 "${CMK_CONFIG_H_IN_NEW_MD5}" CACHE STRING "Md5 of ${CMK_CONFIG_H_IN}" FORCE)
> > +    endif ()
> 
> Is this really needed? I mean, does regenerating this file cause unnecessary recompilations of other files?

The configure_file function and make use just timestamp to check if something need to be done, so without this the cmakeconfig.h will always be touched when you do some change that requires a cmake re-run like adding a file or just touching a cmake file, causing a full WebKit recompilation when just a single file compilation and some link operations would be enough.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #38)
> (In reply to comment #37)
> > The amount of manual parsing we end up doing for those FeatureFoo.config files also seems to indicate we can simplify things. Related point: right now, if one adds a new feature to WebKit, it is necessary to edit cmakeconfig.h.cmake and WebKitFeatures.cmake; if we drop WebKitFeatures.cmake and use Feature<Port>.config instead, does it mean one will have to edit N * <CMake-based ports> files?
>
> Not really, if you add a feature you will notify webkit-dev about it and someone will edit just one Feature<Port>.config file adding the flag if such flag is useful for the port. I'm open for new ideas and for sure there are better ways to do this, however I still believe that this patch improves the current situation.

Isn't this quite a big change compared to the current practice? Right now, not only is one supposed to announce new features in mailing list but also edit each port's build system to add the new flags. If I understood it correctly, you're proposing that the developers stop doing that and port maintainers edit their build systems instead?

I'm also interested in knowing your position on the other topics I mentioned in my previous comment before that one above before offering possible suggestions.

Thanks!

[Laszlo Gombos]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > The amount of manual parsing we end up doing for those FeatureFoo.config files also seems to indicate we can simplify things. Related point: right now, if one adds a new feature to WebKit, it is necessary to edit cmakeconfig.h.cmake and WebKitFeatures.cmake; if we drop WebKitFeatures.cmake and use Feature<Port>.config instead, does it mean one will have to edit N * <CMake-based ports> files?
> >
> > Not really, if you add a feature you will notify webkit-dev about it and someone will edit just one Feature<Port>.config file adding the flag if such flag is useful for the port. I'm open for new ideas and for sure there are better ways to do this, however I still believe that this patch improves the current situation.
> 
> Isn't this quite a big change compared to the current practice? Right now, not only is one supposed to announce new features in mailing list but also edit each port's build system to add the new flags. If I understood it correctly, you're proposing that the developers stop doing that and port maintainers edit their build systems instead?

I agree with Hugo on this particular point. At the time when feature are introduced, they are typically disabled for all port (with the potential exception of a single port which is driving the change).

When other ports follow they typically need to make the corresponding code and test expectation changes and I think it make sense to use the same commit to introduce the flag for the port.

This would indeed a change to the current process that needs to be announced and documented but that should not stop the change. This is a good change for the process IMHO.

As a general note this work would need to be reevaluated if in fact a single _dominant_ build system emerges for WebKit - as it has been just recently discussed on WebKit-dev.

[Hugo Parente Lima]

> The idea that we sort of disregard the definitions in FeaturesList.pm in favor of what we define in the hand-crafted FeatureFoo.config files doesn't sound that good to me: I agree with dydx's points in comment #29; if the biggest problem is with the "options that are not real options for a given port", I think it would make sense to improve our webkitperl infrastructure not to show those.

IMO better change only the input of a program rather than the logic every time the scenario change, this is why I did change the FeatureList.pm, to feed the utopia of never touch it again, after all this patch could be considered a improvement to webkitperl to not show "fake" options ;-).
 
> This also makes me think of two distinct groups we need to take into consideration: WebKit developers, who use build-webkit and want to have as many features as possible enabled, and downstream (end users, packagers etc), who prefer only stable features enabled by default. If we generate defaults for both groups from the same file, I'm afraid we'll end up with either developers (and bots) building ports with fewer features enabled (and thus reducing our coverage) or downstreams having to find out for themselves that besides the usual CMake stuff we also happen to have some .config files hidden somewhere which indicates which features are enabled and disabled by default.

I think this isn't a real use case, developers use the common defaults plus minor variations depending on what he's working on, we will never cover all configurations (ok, is mathematically possible =]), packagers also tend to use the same common defaults, minor variations can happen due to security, bugs on some dependency, etc. What I want to say is: better to have just a common set of default flags used by the majority rather than various sets of defaults not strictly used by anyone.


> > Source/cmake/WebKitFeatures.cmake:56
> > +    set(CMK_CONFIG_H_IN "${CMAKE_BINARY_DIR}/cmakeconfig.h.in")
> 
> I'd rather follow our general habit of not abbreviating things and use a name such as "CMAKE_CONFIG_H_IN" here.

I agree, CMK is ugly.

Now I guess I replied all points :-), thanks and sorry for the "quantized" reply

[Hugo Parente Lima]

Abandoning this in favor of something like https://bugs.webkit.org/show_bug.cgi?id=110717 that require less changes on build system plus some way to remove cmakeconfig.h.