Bug 110717 – [EFL] Consolidate defaults for feature flags

• Component: Platform
• Status: RESOLVED WONTFIX

[Laszlo Gombos]

Static (e.g. not dependent on e.g. environment sensing) defaults for feature defines for the efl port are in various places:
 - FeatureDefines.h (C)
 - Tools/Scripts/webkitperl/FeatureList.pm (Perl)
 - Source/cmake/OptionsEfl.cmake (CMake)

The current design leads to inconsistencies and bugs like this one - bug 110715.

This bug is part of a bigger refactoring effort to manage feature defines, but there should be an immediate win just from having the default values on a single place for EFL.

[Laszlo Gombos]

Created attachment 189994 [details]
1st take

I find it also valuable to have a place where the _delta_ between the default project settings and the EFL port is defined - this is now FeatureDefineEfl.h.
This list is used to _compute_ the full list, but the full list does not need to be specified for each port any more.

[Hugo Parente Lima]

I liked this approach, less build system dependent and more clean.

In the future would be nice to have all cmake based ports using something like this to set their default features.

I know this patch is only for EFL, but thinking about the future my only question is regarding to ports not using gcc and using cmake like WinCE (Blackberry?) and the gcc preprocessor call plus grep used in the patch.

[Laszlo Gombos]

Created attachment 190187 [details]
initialize disabled (0 value) features as well not just enabled (1 value)

Thanks for Hugo for spotting this.

[Laszlo Gombos]

(In reply to comment #2)

> I know this patch is only for EFL, but thinking about the future my only question is regarding to ports not using gcc and using cmake like WinCE (Blackberry?) and the gcc preprocessor call plus grep used in the patch.

We seems to be using the preprocessor and grep in WebKitHelpers.cmake already - but I do not know if this is actually executed for WinCE:
EXEC_PROGRAM("${CMAKE_CXX_COMPILER} -E -Wp,-dM - < /dev/null | grep '#define __VERSION__'

I do think that we should make gcc more generic and we could also easily replace grep with perl/pyton as well. I prefer to make those changes as we introduce this for other ports and right now I do not have any means to test it. It also might be that we use these tools in other build steps as well (e.g. generating bindings) in which case there are assumed to be available already (e.g. via Cygwin or other means) on Win as well.

[Hugo Parente Lima]

Comment on attachment 190187 [details]
initialize disabled (0 value) features as well not just enabled (1 value)

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

We may go further in the future and generate the @features dictionary based on values returned by the GCC preprocessor call, so this file would never be touched again.

> Tools/Scripts/build-webkit:93
> +    my $port = "EFL"."__";

There is a function called "cmakeBasedPortName()", may be better to use it if we think in a future where other ports will use this same mechanism.

IMO the "__" would go to the gcc command line string too, e.g. "-DBUILDING_${port}__".

[WebKit Review Bot]

Attachment 190187 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FeatureDefines.h', u'Source/WTF/wtf/Platform.h', u'Source/WTF/wtf/efl/FeatureDefinesEfl.h', u'Source/cmake/OptionsEfl.cmake', u'Tools/ChangeLog', u'Tools/Scripts/build-webkit', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
Source/cmake/OptionsEfl.cmake:58:  Use lowercase command "string"  [command/lowercase] [5]
Source/cmake/OptionsEfl.cmake:59:  Use lowercase command "string"  [command/lowercase] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Laszlo Gombos]

Created attachment 192389 [details]
rebase

[Gyuyoung Kim]

Comment on attachment 192389 [details]
rebase

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

LGTM otherwise.

> Source/cmake/OptionsEfl.cmake:7
> +EXEC_PROGRAM("gcc -E -P -dM -DBUILDING_EFL__ -I ${CMAKE_SOURCE_DIR}/Source/WTF ${CMAKE_SOURCE_DIR}/Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ [01]$\\|^#define WTF_USE_\\w\\+ [01]$' | cut -d' ' -f2-3 | sort" OUTPUT_VARIABLE DEFINITIONS)

As we talk on irc, I think it would be good to move to WebKitHelpers.cmake when other ports want to use this command.

> Source/cmake/OptionsEfl.cmake:63
>  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON)

It looks Bug 110291 was cloased. So, I wonder how to move this feature definitions.

[Laszlo Gombos]

Created attachment 193455 [details]
move the computation out to a macro

[Hugo Parente Lima]

LGTM, I plan to push this very same idea on Nix port after seeing this patch landed.

[Gyuyoung Kim]

Comment on attachment 193455 [details]
move the computation out to a macro

LGTM. I hope remained feature definitions are going to be moved to from OptionEfl.cmake to FeatureDefinesEfl.h.

[Gyuyoung Kim]

Comment on attachment 193455 [details]
move the computation out to a macro

However, someone else might have a final look, probably, Rafael ?

[Gyuyoung Kim]

If there is no comment anymore, I think this patch can be landed.

[Hugo Parente Lima]

Comment on attachment 193455 [details]
move the computation out to a macro

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

> Source/cmake/OptionsEfl.cmake:57
>  WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_LLINT ON)

Why ENABLE_LLINT, ENABLE_MEMORY_SAMPLER and ENABLE_ACCESSIBILITY weren't removed from OptionsEfl.cmake?

[Daniel Bates]

Comment on attachment 193455 [details]
move the computation out to a macro

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

> Tools/Scripts/build-webkit:96
> +    my $Definitions = `gcc -E -P -dM -D$portDefine -I Source/WTF Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ \[01\]\$\\|^#define WTF_USE_\\w\\+ \[01\]\$' | cut -d' ' -f2-3`;

I wish there was a better way to do this. We follow the WebKit Code Style Conventions for Perl code. So, variable names should be in CamelCase and begin with a lowercase letter.

> Tools/Scripts/build-webkit:99
> +    my %FeatureDefaults;    

As mentioned above, variable names should be in CamelCase and begin with a lowercase letter.

> Tools/Scripts/build-webkit:100
> +    foreach (split(/\n/, $Definitions)) {

Ditto.

> Tools/Scripts/build-webkit:101
> +        my @MacroDefinition = split(/ /);

Ditto.

> Tools/Scripts/build-webkit:102
> +        $FeatureDefaults{$MacroDefinition[0]} = $MacroDefinition[1];

Ditto.

> Tools/Scripts/build-webkit:106
> +        if (exists $FeatureDefaults{$_->{define}}) {

Ditto.

[Daniel Bates]

Comment on attachment 193455 [details]
move the computation out to a macro

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

> Tools/Scripts/build-webkit:93
> +    my $port = "EFL";

Do you anticipate making use of this local variable in the near future? Otherwise, I suggest we inline its value into the definition of $portDefine (below) since that is the only expression that references $port inside this if-block.

> Tools/Scripts/build-webkit:98
> +    # hash table with macro names and values

This comment doesn't add much value as it states what the code below does. I suggest you either elaborate further or remove this comment.

> Tools/Scripts/build-webkit:107
> +            $_->{default} = $FeatureDefaults{$_->{define}};

The variable name $FeatureDefaults should be written in CamelCase and begin with a lowercase letter.

[Hugo Parente Lima]

Comment on attachment 193455 [details]
move the computation out to a macro

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

>> Tools/Scripts/build-webkit:93
>> +    my $port = "EFL";
> 
> Do you anticipate making use of this local variable in the near future? Otherwise, I suggest we inline its value into the definition of $portDefine (below) since that is the only expression that references $port inside this if-block.

I plan to use this code on Nix (an out of trunk port), so would be better to use cmakeBasedPortName() function instead of the "EFL" string to turn the code more generic.

[Hugo Parente Lima]

Created attachment 201895 [details]
Patch

[Hugo Parente Lima]

I did talk with Laszlo on IRC to take over this patch because he has another priorities at the moment, so here's the patch.

The only difference to the original patch is that this version doesn't use the features defined in FeatureList.pm at all, it relies only on what gcc found, this may be a initial step path to a future removal of FeatureList.pm, cmakeconfig.h, etc...

[EFL EWS Bot]

Comment on attachment 201895 [details]
Patch

Attachment 201895 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/483051

[EFL EWS Bot]

Comment on attachment 201895 [details]
Patch

Attachment 201895 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/480233

[Build Bot]

Comment on attachment 201895 [details]
Patch

Attachment 201895 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/471780

New failing tests:
editing/pasteboard/4076267-3.html

[Build Bot]

Created attachment 201908 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Hugo Parente Lima]

Created attachment 202083 [details]
Patch

Now with shadow dom disabled

[EFL EWS Bot]

Comment on attachment 202083 [details]
Patch

Attachment 202083 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/486435

[EFL EWS Bot]

Comment on attachment 202083 [details]
Patch

Attachment 202083 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/487424

[Laszlo Gombos]

Comment on attachment 202083 [details]
Patch

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

> Tools/Scripts/build-webkit:121
> +        # Fix flag names not following any naming rule.
> +        if ($optionName eq "dialog-element") {
> +            $optionName = "dialog";
> +        } elsif ($optionName eq "datalist-element") {
> +            $optionName = "datalist";
> +        } elsif ($optionName eq "details-element") {
> +            $optionName = "details";
> +        } elsif ($optionName eq "meter-element") {
> +            $optionName = "meter-tag";
> +        }

This will be challenging to maintain in the future. Can we instead fix the optionNames to be consistent ?

[Hugo Parente Lima]

(In reply to comment #27)
> (From update of attachment 202083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202083&action=review
> 
> > Tools/Scripts/build-webkit:121
> > +        # Fix flag names not following any naming rule.
> > +        if ($optionName eq "dialog-element") {
> > +            $optionName = "dialog";
> > +        } elsif ($optionName eq "datalist-element") {
> > +            $optionName = "datalist";
> > +        } elsif ($optionName eq "details-element") {
> > +            $optionName = "details";
> > +        } elsif ($optionName eq "meter-element") {
> > +            $optionName = "meter-tag";
> > +        }
> 
> This will be challenging to maintain in the future. Can we instead fix the optionNames to be consistent ?

Yes, we can, but maybe would be better to do the change in another patch just to have time to announce the change on webkit-dev, what do you think?

The EWS (and my computer) is blaming about -Werror=maybe-unitialized, it may be a compiler error not related with this patch.

[Hugo Parente Lima]

Created attachment 202740 [details]
Patch

Added missing WTF_USE_TILED_BACKING_STORE to options list.

[Hugo Parente Lima]

Created attachment 203957 [details]
Patch

Rebased, removed command line renames, added new CSS_SHAPES flag.

[Hugo Parente Lima]

Created attachment 204637 [details]
Patch

[Hugo Parente Lima]

New version.

- Rebased to solve conflicts on FeatureLists.pm
- Make Efl port free from the infamous cmakeconfig.h
- Feature summary reflect the current state, nowadays it don't in some cases.

Comments appreciated, if everything goes well we can forget about cmakeconfig.h and FeatureLists.pm :-)

[Hugo Parente Lima]

Created attachment 204722 [details]
Patch

rebase

[Laszlo Gombos]

(In reply to comment #32)
> New version.

> - Make Efl port free from the infamous cmakeconfig.h

I think it would make it easier to review if you did this step as a follow-up patch and not combined into the patch that is already quite big in size.

[Martin Robinson]

Comment on attachment 204722 [details]
Patch

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

Looks pretty good to me. I have a couple questions though.

> Source/cmake/WebKitFeatures.cmake:20
> +    if (${ARGC} EQUAL 1)

If the port name isn't read, why pass the port name?

> Source/cmake/WebKitFeatures.cmake:22
> +        EXEC_PROGRAM("gcc -E -P -dM -DBUILDING_${ARGV0}__ -I ${CMAKE_SOURCE_DIR}/Source/WTF ${CMAKE_SOURCE_DIR}/Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ [01]' | cut -d' ' -f2-3 | sort" OUTPUT_VARIABLE DEFINITIONS)

Hrm. CMake is a platform-independent build system, so maybe it makes sense to do this in a platform independent way? Is the idea with the preprocessor run that you want to combine compiler flags with the #defines in the file?

> Source/cmake/WebKitFeatures.cmake:30
> +        set(FEATURES_GOT_FROM_HEADERS ON)

English nit: GET_FEATURES_FROM_HEADERS

> Tools/Scripts/build-webkit:96
> +    my $definitions = `gcc -E -P -dM -D$portDefine -I Source/WTF Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ \[01\]\$' | cut -d' ' -f2-3`;

Not sure if I understand why this has to happen here and in the cmake file...

[Hugo Parente Lima]

Hi,

Thanks for take a look at the patch.

(In reply to comment #35)
> (From update of attachment 204722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204722&action=review
> 
> Looks pretty good to me. I have a couple questions though.
> 
> > Source/cmake/WebKitFeatures.cmake:20
> > +    if (${ARGC} EQUAL 1)
> 
> If the port name isn't read, why pass the port name?

The port name is read on ${ARGV0}, the idea of this is just to keep backward compatibility with other ports using cmake but not getting the feature flags from header files, if all port agree with this approach we can get rid of all those "if(FEATURES_GOT_FROM_HEADERS)" and keep the things simpler.
 
> > Source/cmake/WebKitFeatures.cmake:22
> > +        EXEC_PROGRAM("gcc -E -P -dM -DBUILDING_${ARGV0}__ -I ${CMAKE_SOURCE_DIR}/Source/WTF ${CMAKE_SOURCE_DIR}/Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ [01]' | cut -d' ' -f2-3 | sort" OUTPUT_VARIABLE DEFINITIONS)
> 
> Hrm. CMake is a platform-independent build system, so maybe it makes sense to do this in a platform independent way? Is the idea with the preprocessor run that you want to combine compiler flags with the #defines in the file?

Yes, it's. I also doesn't like too much this gcc dependent approach, but I think this can be replaced by a perl script later mainly because I guess WinCE doesn't have g++.
 
> > Source/cmake/WebKitFeatures.cmake:30
> > +        set(FEATURES_GOT_FROM_HEADERS ON)
> 
> English nit: GET_FEATURES_FROM_HEADERS

The meaning of this variable is to flag that the features were got from header files, not a flag to tell cmake to get the features from header files. My english grammar skills are far away from perfect, so I would ask if with this variable meaning in mind the english nit still valid.
 
> > Tools/Scripts/build-webkit:96
> > +    my $definitions = `gcc -E -P -dM -D$portDefine -I Source/WTF Source/WTF/wtf/Platform.h | grep '^#define ENABLE_\\w\\+ \[01\]\$' | cut -d' ' -f2-3`;
> 
> Not sure if I understand why this has to happen here and in the cmake file...

This happen here to build-webkit script be able to build a complete list of features when you type "build-webkit --efl --help" and be able to pass the correct -DENABLE_FOOBAR=1 or -DENABLE_FOOBAR=0 to cmake.

This also happen on cmake, so cmake can build the cmake options (call option function for every possible feature), so -DENABLE_FOOBAR=1 will reflect on cmake scripts as expected.

Nowadays all this is made by hand, we keep a list of features on FeatureList.pm, so build-webkit script can list the features on --help, etc... and we keep another list in WebKitFeatures.cmake overwritten by another one on OptionsEfl.cmake, so cmake can add the options, etc, all those lists *must* be in sync, besides all that we also must have all options (feature flags) listed on cmakeconfig.h or the changes (-DENABLE_FOOBAR=x) will not reflect on the build itself. Pretty complicated, verbose, bureaucratic and error prone IMO.

[Laszlo Gombos]

(In reply to comment #36)

> > Hrm. CMake is a platform-independent build system, so maybe it makes sense to do this in a platform independent way? Is the idea with the preprocessor run that you want to combine compiler flags with the #defines in the file?
> 
> Yes, it's. I also doesn't like too much this gcc dependent approach, but I think this can be replaced by a perl script later mainly because I guess WinCE doesn't have g++.

Alternatively all compilers (gcc, clang, MSVC, etc) can be invoked just to run the pre-processor - usually with the -E or /E option passed to the compiler - and we already rely on the compiler for the build.

I find it a good approach to start with gcc first and add other environments later. According to the tile of this bug, this is initially just for EFL port and the EFL port only supports Linux and gcc at the moment.

[Gyuyoung Kim]

Comment on attachment 204722 [details]
Patch

Cleared review? from attachment 204722 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

[Hugo Parente Lima]

(In reply to comment #38)
> (From update of attachment 204722 [details])
> Cleared review? from attachment 204722 [details] [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

Hi, Nix is already the used by this patch to evaluate the feature flags and it works very well, if there are interest into using this approach on Efl I could spend some time moving the CMake functions used by Nix to some general place and make a patch for EFL, otherwise I don't plan to waste my time on this.

Thanks, feel free to tag the bug as WONTFIX if there's no interest on this.

[Gyuyoung Kim]

I'm not interested in this patch now. Closed.