| Summary: | Add values for all features to Qt's features.pri | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
| Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ossy, vestbo, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | 85611 | ||||||
| Bug Blocks: | 85456 | ||||||
| Attachments: |
|
||||||
|
Description
Eric Seidel (no email)
2012-05-06 14:19:10 PDT
Created attachment 140434 [details]
Patch
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 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. 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 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 on attachment 140434 [details] Patch Clearing flags on attachment: 140434 Committed r116294: <http://trac.webkit.org/changeset/116294> All reviewed patches have been landed. Closing bug. |