Bug 85746 - Add values for all features to Qt's features.pri
Summary: Add values for all features to Qt's features.pri
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 85611
Blocks: 85456
  Show dependency treegraph
 
Reported: 2012-05-06 14:19 PDT by Eric Seidel (no email)
Modified: 2012-05-07 04:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.74 KB, patch)
2012-05-06 14:23 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-05-06 14:19:10 PDT
Add values for all features to Qt's features.pri
Comment 1 Eric Seidel (no email) 2012-05-06 14:23:03 PDT
Created attachment 140434 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-06 14:24:46 PDT
I would like someone with knowledge of the Qt port to review this and make sure that I'm not disabling any features Qt actually wants on.

It's important that we get this list correct, as this is a representation of what will appear in Features.py (and thus be used to generate the defaults for build-webkit --help, etc.)

You can see the current Features.py listed here:
https://github.com/eseidel/webkit/compare/master...features
Comment 3 Eric Seidel (no email) 2012-05-06 14:28:23 PDT
I can also tweak the PRI-generator to not output values for features which are disabled... but I'd rather stick with a full list if that's compatible with how the Qt build system works.

This list of enable/disables would ideally be "policy-level" decisions -- features which the "qt port" believes should be on/off.  Individual builders of the Qt port can of course modify this list (as presumably folks do), but in my vision this list (as well as the Features.py list) should be the high-level view of what "should be" on vs. off.
Comment 4 Eric Seidel (no email) 2012-05-06 14:29:00 PDT
Anyway, I very much welcome your feedback, and do not wish to presume too much about the workings of your build system.  Let me know if this can work for you. :)
Comment 5 Tor Arne Vestbø 2012-05-07 03:56:08 PDT
Comment on attachment 140434 [details]
Patch

The patch looks good! 

Having an exhaustive list in features.pri is fine, the way things work right now is that we:

 1. Parse command line options (that as passed to qmake from build-webkit, eg --enable-foo turns into DEFINES+=ENABLE_FOO=1
 2. Do dynamic feature checking based on optional dependencies (which might add eg, ENABLE_VIDEO=1 to DEFINES, if the right libs are available). 
 3. Fill in any un-set features from the defaults in features.pri

This is a bit reverse, but that' just how it has to be done right now due to qmake weirdness :)

So basically, if the dynamic check for eg. video fails, it will then end up using the default in features.pri, which is ENABLE_VIDEO=0, since it's off by default.

So for the Qt port (and Chromium) the list in Features.py really is a tri-state (on/off/auto), with off/auto baked into one. I think that's fine as long as the list in Features.py is not be used to say "oh, no port has this feature enabled, we can remove it", since we might actually enable it dynamically. So perhaps the script should have some sort of warning about making decisions solely based on the output of the script?
Comment 6 WebKit Review Bot 2012-05-07 04:12:25 PDT
Comment on attachment 140434 [details]
Patch

Clearing flags on attachment: 140434

Committed r116294: <http://trac.webkit.org/changeset/116294>
Comment 7 WebKit Review Bot 2012-05-07 04:12:37 PDT
All reviewed patches have been landed.  Closing bug.