Summary: | [EFL] Consolidate defaults for feature flags | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||||||||||||||||||
Component: | Platform | Assignee: | Hugo Parente Lima <hugo.lima> | ||||||||||||||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, eflews.bot, gyuyoung.kim, gyuyoung.kim, hugo.lima, kenneth, ojan.autocc, paroga, rakuco, rniwa, sergio, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | 117254 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Laszlo Gombos
2013-02-24 16:22:12 PST
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.
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. Created attachment 190187 [details]
initialize disabled (0 value) features as well not just enabled (1 value)
Thanks for Hugo for spotting this.
(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. 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}__". 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.
Created attachment 192389 [details]
rebase
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. Created attachment 193455 [details]
move the computation out to a macro
LGTM, I plan to push this very same idea on Nix port after seeing this patch landed. 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.
Comment on attachment 193455 [details]
move the computation out to a macro
However, someone else might have a final look, probably, Rafael ?
If there is no comment anymore, I think this patch can be landed. 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? 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. 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. 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. Created attachment 201895 [details]
Patch
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... 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 Comment on attachment 201895 [details] Patch Attachment 201895 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/480233 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 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
Created attachment 202083 [details]
Patch
Now with shadow dom disabled
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 Comment on attachment 202083 [details] Patch Attachment 202083 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/487424 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 ? (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. Created attachment 202740 [details]
Patch
Added missing WTF_USE_TILED_BACKING_STORE to options list.
Created attachment 203957 [details]
Patch
Rebased, removed command line renames, added new CSS_SHAPES flag.
Created attachment 204637 [details]
Patch
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 :-) Created attachment 204722 [details]
Patch
rebase
(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. 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... 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. (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. 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. (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. I'm not interested in this patch now. Closed. |