Bug 85456 – WebKit should have an easy way to add/remove FEATURE_DEFINES for all ports

• Component: New Bugs
• Status: NEW

[Eric Seidel (no email)]

WebKit should have an easy way to add/remove FEATURE_DEFINES for all ports

[Eric Seidel (no email)]

Created attachment 139954 [details]
wip patch

[Eric Seidel (no email)]

The beauty (in my mind) of a solution like this is that it does not mandate adoption.  Users don't have to use this script to edit these files (they can continue to edit them like always), but those who wish to use this script can edit all the files at once, automatically. :)

Furthermore, this gives us a central database of what ports use what features.  We can learn interesting things, like the fact that the WinCairo port has a bunch of features disabled that it probably doesn't want to... or that Mac appears to be the only port using the NOTIFICATION stuff, or have a better idea of how many features are chromium-only (once we add auto-generation of Chromium's feature_defines.gypi).

Another big-win comes when we automate generation of the FEATURE_DEFINES code used by DerivedSources.make, since currently its even more complicated to add an ENABLE_ if you want to be able to use it from an idl or html.css. :(

[Eric Seidel (no email)]

Comment on attachment 139954 [details]
wip patch

The patch is much more interesting when you see what it generates.  I'll post that patch once bug 85454 lands.

[Eric Seidel (no email)]

I'm interested in working with the Gtk, Qt, and CMake maintainers to add auto-generation for those FEATURE_DEFINES lists as well.

Qt uses Tools/qmake/mkspecs/features/features.prf (which does *not* look well suited for auto-generation)

CMake ports use Source/cmake/WebKitFeatures.cmake (which should be easy to autogenerate, especially if some of the header/footer are split out into a separate file).

I'm not sure what Gtk uses.

build-webkit's --enable-foo option list should also be easy to generate as a perl module which build-webkit can then include.

HTML_FLAGS generation should also be easy to automate/autogenerate:
http://trac.webkit.org/browser/trunk/Source/WebCore/DerivedSources.make#L815

[Eric Seidel (no email)]

Created attachment 139957 [details]
diff when running the script against TOT

Note how the script catches the fact that ENABLE_LEGACY_CSS_VENDOR_PREFIXES is defined in WebCore/Configurations/FeatureDefines.xcconfig, but not in the other *.xcconfig files (a common mistake!)

[Tor Arne Vestbø]

First of all, let me say that I absolutely applaud the effort to make things easier in these areas, and as you say, this is certainly better than what we have today. I do however have some questions:

1. build-webkit already has a similar list to the one in Features.py, which is the somewhat de-facto list of feature-defines-that-all-ports-should-care-about (as far as I can tell). By duplicating this list in Features.py we're slightly complicating the issue for those tho manually add defines. Is there some way we can avoid that? What happens when someone adds a define manually, but forgets to update Features.py? Should the next author who runs this script and notices that the diff looks off do the sync?

2. The existing list in build-webkit allows for dynamically evaluating the default-values of the defines, something we take advantage of in the Qt port since the defines can vary depending on the platform and optional dependencies. Is this something you've planned for?

3. A bunch of features are also defined in wtf/Platform.h, what's the relationship between things that go in build-webkit/Features.py, and Platform.h? Should Platform.h only be used for features that are specific to a given port?

Regarding the Qt-port's features.prf, it's can't be auto-generated wholesale (due to the dynamic evaluation of which defines to enable), but a large percentage of the defines are still static, and we could certainly include an auto-generated list of those static defines. How do you see this expressed in Features.py (that a subset of the features are defined, either enabled or disabled, but some are left out)?

Anyways, I'm happy to help make this work for the Qt port as well.

[Eric Seidel (no email)]

(In reply to comment #6)
> First of all, let me say that I absolutely applaud the effort to make things easier in these areas, and as you say, this is certainly better than what we have today. I do however have some questions:
> 
> 1. build-webkit already has a similar list to the one in Features.py, which is the somewhat de-facto list of feature-defines-that-all-ports-should-care-about (as far as I can tell). By duplicating this list in Features.py we're slightly complicating the issue for those tho manually add defines. Is there some way we can avoid that?

Yup.  I'm very close to being able to generate build-webkit's list exactly. :)  I'll post an updated patch once I have the diff small enough.

> What happens when someone adds a define manually, but forgets to update Features.py? Should the next author who runs this script and notices that the diff looks off do the sync?

I think if I'm successful, such a diff will not happen in practice.   Adding an ENABLE_ by hand is really a ton of work... at least if you want to do it right.  If we go with this solution, all these files will have a "do not edit." warning at the top, with instructions on how to regenerate them.  In the case the someone needs to edit a file by hand, or does because they failed to read the warning, it's fine, these syncs have proven very easy to deal with so far. :)

> 2. The existing list in build-webkit allows for dynamically evaluating the default-values of the defines, something we take advantage of in the Qt port since the defines can vary depending on the platform and optional dependencies. Is this something you've planned for?

Qt's logic for such appear to be entirely in Tools/qmake/mkspecs/features/features.prf.  build-webkit makes its decisions soley based on which port you're building.  I think this setup is very useful for generating feature *defaults* and ports which want to do special handling based on library detection, etc. will still need to do that in their own build files.  Those build files should just read from some central list of defaults (which this script can help you generate).

> 3. A bunch of features are also defined in wtf/Platform.h, what's the relationship between things that go in build-webkit/Features.py, and Platform.h? Should Platform.h only be used for features that are specific to a given port?

Yes.  We've discussed moving ENABLE_ macros out of Platform.h in the past.  We don't have to do that to adopt this solution, but we'll eventually want to.  I'm being careful in my editing to make sure that I'm syncing my feature "database" with the information in all the existing build files, including Platform.h.

> Regarding the Qt-port's features.prf, it's can't be auto-generated wholesale (due to the dynamic evaluation of which defines to enable), but a large percentage of the defines are still static, and we could certainly include an auto-generated list of those static defines. How do you see this expressed in Features.py (that a subset of the features are defined, either enabled or disabled, but some are left out)?

The current mental model I'm shooting for is one where this script will generate all of these "default" lists, and most ports will use the defaults wholesale.  Some ports, like Qt and Chromium, which want to build in lots of different configurations/environments will need to read in the defaults list from some build file of their choosing (which this script can easily generate) and then modify it based on the configuration they find themselves in.  This build file split will be needed for both features.prf (Qt) and feature_defines.gypi (chromium), as well as possibly the CMake files.  If you're willing to split the static parts of features.prf out from the dynamic parts, that would make it possible for mee to generate the feature_defaults.prf very easily. :)

> Anyways, I'm happy to help make this work for the Qt port as well.

Thanks!  The above mentioned split is the first step for Qt.

[Tor Arne Vestbø]

(In reply to comment #7)
> Yup.  I'm very close to being able to generate build-webkit's list exactly. :)  I'll post an updated patch once I have the diff small enough.

Sweet!

> > 2. The existing list in build-webkit allows for dynamically evaluating the default-values of the defines, something we take advantage of in the Qt port since the defines can vary depending on the platform and optional dependencies. Is this something you've planned for?
> 
> Qt's logic for such appear to be entirely in Tools/qmake/mkspecs/features/features.prf.  build-webkit makes its decisions soley based on which port you're building. 

build-webkit also calls out to "qmake features.pro CONFIG+=compute_defaults", which will let us update the @features list's defaults based on the actual platform. This is useful for two reasons:

 1. running build-webkit --help will show the correct defaults for your current setup
 2. build-webkit (though webkitdirs.pm), will only pass overridden features to qmake, eg DEFINES+=ENABLE_VIDEO=0, not every single feature-define, which makes it easier to see when you're building what sort of modifications you did to the build config. (The full config is still available in .webkit.config in the root build dir).

But, from looking at bug 85548 this would still work, as we would get a static list from the new perl module, and still run qmake to compute the dynamic defaults.

> The current mental model I'm shooting for is one where this script will generate all of these "default" lists, and most ports will use the defaults wholesale.  Some ports, like Qt and Chromium, which want to build in lots of different configurations/environments will need to read in the defaults list from some build file of their choosing (which this script can easily generate) and then modify it based on the configuration they find themselves in.  This build file split will be needed for both features.prf (Qt) and feature_defines.gypi (chromium), as well as possibly the CMake files.  If you're willing to split the static parts of features.prf out from the dynamic parts, that would make it possible for mee to generate the feature_defaults.prf very easily. :)

Ah, I see, I think that should be workable for the Qt port, I'll have a look!

[Tor Arne Vestbø]

(In reply to comment #8)
> Ah, I see, I think that should be workable for the Qt port, I'll have a look!

Tools/qmake/mkspecs/features/features.pri is now ready to be auto-generated.

[Eric Seidel (no email)]

OK.  Have Qt generation up and running.  Gtk is the only big one left.  (CMake will require some trivial refactoring, but I don't want to ask them to do that until the rest lands.)  If someone from the Gtk port would like to help refactor the Gtk system similar to how Tor did with Qt, I'm happy to include Gtk support in the patch I post for review (probably Monday).

[Zan Dobersek]

I'll try to describe the Gtk port's behavior, but would like to get a rubber-stamp on this comment from a senior port maintainer (Martin or Xan who are already CC-ed).

The Gtk port does not really have an equivalent to Qt's features.prf. The only way of dynamically enabling an disabling features at compile-time is through the configure script. This is generated out of configure.ac, but I would not recommend auto-generating any part of this file as there is more logic required for specific features to get them enabled (additional libraries etc.). Another barrier is that it wouldn't be appropriate to have all the possible features listed there as many of these features are not supported yet by the Gtk port and given that the file is basically the gateway to configuring a release version of the product this would only confuse the users. Generally, though admittedly not in all cases, the features present in these files are those that are either supported or in progress of being supported. Because of that it is best that the feature-enabling configuration options are maintained manually.

The build-webkit script simply translates the features to configuration flags and later in the process calls the configure script.

In compilation, (more or less supported) features enabled via the configure script are then each checked in Source/WebCore/GNUmakefile.am, adding the feature to the feature defines and the cppflags if enabled. This is a part of logic that could be split into a GNUmakefile.features.am file and be autogenerated, but it shouldn't stop you from continuing on working on the patch (basically it shouldn't be a compilation-breaker). Currently these checks are maintained parallel with the corresponding feature in configure.ac.

Overall, from the current work, the Gtk port depends most on a properly-generated feature list that's used in build-webkit. The feature configuration options in configure.ac are maintained manually and I think they would be preferred to stay that way. A possible GNUmakefile.features.am autogeneration can be added later, but it shouldn't be a showstopper at the moment.

[Martin Robinson]

(In reply to comment #11)

> Overall, from the current work, the Gtk port depends most on a properly-generated feature list that's used in build-webkit. The feature configuration options in configure.ac are maintained manually and I think they would be preferred to stay that way. A possible GNUmakefile.features.am autogeneration can be added later, but it shouldn't be a showstopper at the moment.

I think I agree with your assessment. I wonder if this sort of situation would better than what we have now:

1. Most WebKit/WebCore features are controlled via the option autogeneration and are hardcoded in the generated file.
2. configure.ac has a very minimal set of features that override the ones in the autogenerated file. I think it would be useful to trim down the options exposed to the user to a much smaller set.

[Zan Dobersek]

(In reply to comment #12)

I've filed two bugs for these changes:

> I think I agree with your assessment. I wonder if this sort of situation would better than what we have now:
> 
> 1. Most WebKit/WebCore features are controlled via the option autogeneration and are hardcoded in the generated file.

Bug #87127, will block this bug.

> 2. configure.ac has a very minimal set of features that override the ones in the autogenerated file. I think it would be useful to trim down the options exposed to the user to a much smaller set.

Bug #87126, not that much related to this bug.

[Csaba Osztrogonác]

(In reply to comment #10)
> OK.  Have Qt generation up and running.  Gtk is the only big one left.  (CMake will require some trivial refactoring, but I don't want to ask them to do that until the rest lands.)  If someone from the Gtk port would like to help refactor the Gtk system similar to how Tor did with Qt, I'm happy to include Gtk support in the patch I post for review (probably Monday).

Is there any update on this bug? There are zillion new, but disabled
features on Qt, because authors don't know if they should add them
to features.pri.

It would be great if we don't have to find new features manually.

[Thiago Marcos P. Santos]

For every feature, CMake has an entry at Source/cmake/WebKitFeatures.cmake and  Source/cmakeconfig.h.cmake. Features that are enable for every CMake port are marked as ON at WebKitFeatures.cmake, which are just a few.

After that, every port can override these settings at cmake/Options[Efl,BlackBerry,WindowsCE,etc].cmake. This is how the CMake port works currently.

We will need to split Options[PortName].cmake into WebKitFeatures[PortName].cmake in order to make it generator friendly, since currently it does more things than just features overrides. 

What I propose is, for every existing feature we add a disabled entry at WebKitFeatures.cmake and a declaration at cmakeconfig.h.cmake. For every feature enabled for a given CMake port, add an enabled entry at WebKitFeatures[PortName].cmake.

I volunteer doing this work if it sounds sane for the other CMake guys.

[Ming Xie]

(In reply to comment #15)
> For every feature, CMake has an entry at Source/cmake/WebKitFeatures.cmake and  Source/cmakeconfig.h.cmake. Features that are enable for every CMake port are marked as ON at WebKitFeatures.cmake, which are just a few.
> 
> After that, every port can override these settings at cmake/Options[Efl,BlackBerry,WindowsCE,etc].cmake. This is how the CMake port works currently.
> 
> We will need to split Options[PortName].cmake into WebKitFeatures[PortName].cmake in order to make it generator friendly, since currently it does more things than just features overrides. 
> 
> What I propose is, for every existing feature we add a disabled entry at WebKitFeatures.cmake and a declaration at cmakeconfig.h.cmake. For every feature enabled for a given CMake port, add an enabled entry at WebKitFeatures[PortName].cmake.
> 
> I volunteer doing this work if it sounds sane for the other CMake guys.

This sounds okay for the BlackBerry port.

[Patrick R. Gansterer]

(In reply to comment #15)
> For every feature, CMake has an entry at Source/cmake/WebKitFeatures.cmake and  Source/cmakeconfig.h.cmake. Features that are enable for every CMake port are marked as ON at WebKitFeatures.cmake, which are just a few.
> 
> After that, every port can override these settings at cmake/Options[Efl,BlackBerry,WindowsCE,etc].cmake. This is how the CMake port works currently.
> 
> We will need to split Options[PortName].cmake into WebKitFeatures[PortName].cmake in order to make it generator friendly, since currently it does more things than just features overrides. 
> 
> What I propose is, for every existing feature we add a disabled entry at WebKitFeatures.cmake and a declaration at cmakeconfig.h.cmake. For every feature enabled for a given CMake port, add an enabled entry at WebKitFeatures[PortName].cmake.
> 
> I volunteer doing this work if it sounds sane for the other CMake guys.

IMHO you should remove the "duplicated" WEBKIT_OPTION_DEFINE vs. WEBKIT_OPTION_DEFAULT_PORT_VALUE then as a whole. If the default values is already in the Features.py i would suggest to generate a list with _all_ possibles features for every CMake port. This means removing the WEBKIT_OPTION_BEGIN() and WEBKIT_OPTION_DEFAULT_PORT_VALUE() macro and replacing it with a WEBKIT_OPTION_DEFINE() and a final WEBKIT_OPTION_PRINT_LIST() (which should go into the central CMakeList.txt again).

So WebKitFeatures.cmake only defines the macros for handling the features, but does not know about any existing feature. Then the WebKitFeatures[PortName].cmake lists all features generated by the python script. This should make it easier to understand the build system.

[Laszlo Gombos]

(In reply to comment #7)
> (In reply to comment #6)

> The current mental model I'm shooting for is one where this script will generate all of these "default" lists, and most ports will use the defaults wholesale.  

Eric, what do you think about using a header file to be the single authoritative input to generate (or perhaps to be) the _default lists_. As you described above ports (or the person running build-webkit with options) can further change/overrule these defaults based on different configurations/environments.

To evaluate the header files from python or from the port specific build systems directly, we could use the C preprocessor that should be available in all platforms WebKit supports.

As an example to get the enabled defaults for a PLATFORM(XXX) just take the output of the following command:

gcc -E -dM -I. -DWTF_PLATFORM_XXX "wtf/Platform.h" | grep "ENABLE_\w\+ 1" | cut -d' ' -f2 | sort

This approach enables:
 - to use cross-platform language to describe the defaults (and not port specific build system).
 - to remove defaults from the build system that are actually static and can not be changed by inspecting the environment WebKit is being built. Martin also expressed a desire for this direction in https://bugs.webkit.org/show_bug.cgi?id=85456#c12 (2. bullet).

See bug 105735 for other benefits and for the enabling patch.

Any feedback on this direction is welcomed.

[Laszlo Gombos]

(In reply to comment #18)
> (In reply to comment #7)
> > (In reply to comment #6)
> 
 
> As an example to get the enabled defaults for a PLATFORM(XXX) just take the output of the following command:
> 
> gcc -E -dM -I. -DWTF_PLATFORM_XXX "wtf/Platform.h" | grep "ENABLE_\w\+ 1" | cut -d' ' -f2 | sort

Note that this technique is already used to populate feature defaults from Platform.h into the build stem for the MAC port.

See out WebCore/DerivedSources.make and check how the defaults for ENABLE_DASHBOARD_SUPPORT and for ENABLE_ORIENTATION_EVENTS are set.

[Mark Rowe (bdash)]

> As an example to get the enabled defaults for a PLATFORM(XXX) just take the output of the following command:
> 
> gcc -E -dM -I. -DWTF_PLATFORM_XXX "wtf/Platform.h" | grep "ENABLE_\w\+ 1" | cut -d' ' -f2 | sort

This sort of approach has proven problematic in the past since the compiler invocation used to extract the value of the define must use the exact same arguments as compilation build phases that use the header file. That's challenging to do correctly with Xcode, particularly when factors such as building for multiple architectures at once or against SDKs is considered.

Moving the defintions out of .xcconfig files would also mean that they can no longer be used in conditional logic within .xcconfig files. We currently use that in at least one place that I see.

[Laszlo Gombos]

(In reply to comment #20)

Perhaps it helps the discussion if I document the mental model I am considering. 

A feature definition can be set (enabled or disabled) by the following entities:

#1./ Developer/package maintainer explicitly setting a flag (e.g. as a build-webkit argument or SDK GUI input).

#2./ Build system (e.g. Xcode, gyp, qmake, cmake rules) setting a flag based on a build or target environment (e.g. cpu architecture, SDK/OS version, available libraries).

#3./ C/C++ source code defaults as it is evaluated by compiler (e.g. based on Platform.h, config.h).

I was hoping to set up the infrastructure so that for all ports the preference order is the same #1 being the highest preference and #3 being the lowest preference. To achieve this the build system should never overrule the request from the developer initiating the build and C/C++ code should always check if a macro is defined before setting a default value.

> Moving the defintions out of .xcconfig files would also mean that they can no longer be used in conditional logic within .xcconfig files. We currently use that in at least one place that I see.

Thanks Mark for helping out. 

You certainly raise a valid point. I think for the particular situation you described (where the flag is dependent on the architecture and/or SDK version) for the Mac port the best solution might be to keep those rules in the .xconfig files as it is now (no change). I assume that for these complex cases "build-webkit --help" already reports false defaults and one would have to kick off a build to determine the value of the feature flag.

The pieces where we could make progress are:

 - Simple cases where the value of the feature flag only depends on the port or maybe even the same for all ports. As an example ENABLE_WORKERS seems to be always set to 1, but one needs to dig a fair amount today to gather this information.

 - In case we introduce a _new_ feature flags one could just introduce the flag to the C/C++ code that is cross-platform with a disabled default. Over time as ports enable the feature the flag might have to be introduced in the port specific build systems as well (or if the port never introduces a particular feature the flag would not have to be introduced in that particular build system).