Summary: | Make WTF public headers use fully-qualified include paths and remove ForwardingHeaders/wtf | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, donggwan.kim, gustavo, levin+threading, lforschler, mrowe, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||
Bug Blocks: | 75673 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2012-03-05 18:50:00 PST
Created attachment 130265 [details]
Patch
This will fail to compile on the EWSes until bug 79960 is landed. I'll re-post the patch after that since this more needs green EWS bubbles than it does detailed code review. This approach is per a private discussion with Mark Rowe. I'm open to other options, but it seems that making these imports fully qualified will simplify all our build systems since they will not need to include each individual wtf subdirectory in their include list. Mac/Win (although the first) are these days outliers in how they import WTF headers. Mac/Win flatten all WTF headers into a single JavaScriptCore private headers directory, and then depend on ForwardingHeaders to map from the fully-qualified names to the JavaScriptCore/Foo.h names. This patch depends on bug 79960 which makes Mac/Win install non-flattened WTF headers into a private directory. Comment on attachment 130265 [details] Patch Attachment 130265 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11832104 Comment on attachment 130265 [details] Patch Attachment 130265 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11836128 Comment on attachment 130265 [details] Patch Attachment 130265 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11838135 Created attachment 130285 [details]
same patch, but now should apply
Comment on attachment 130285 [details] same patch, but now should apply View in context: https://bugs.webkit.org/attachment.cgi?id=130285&action=review > Source/JavaScriptCore/wtf/unicode/wince/UnicodeWinCE.h:30 > -#include "ce_unicode.h" > +#include <wtf/ce_unicode.h> Oops. This looks wrong? Where does ce_unicode.h come from? Sadly WinCE has no EWS. > Source/WebCore/DerivedSources.make:643 > +ifeq ($(shell $(CC) -E -P -dM $(FRAMEWORK_FLAGS) -I$(WTF_HEADERS_INCLUDE_PATH) $(WTF_HEADERS_INCLUDE_PATH)/wtf/Platform.h | grep ENABLE_DASHBOARD_SUPPORT | cut -d' ' -f3), 1) This no longer really needs to use the preprocessor, but it doesn't hurt to. What this line of code really wants is for all ports to have a consistent way of specifying feature defines. :) An EnabledFeatures.in file or something. Created attachment 130288 [details]
Theoretically unbreak wince (assuming anyone even compiles wince these days)
Actually, I think wince would still be broken by this. But as far as I can tell the WinCE port is dead, so I'm not going to bother updating. Comment on attachment 130288 [details] Theoretically unbreak wince (assuming anyone even compiles wince these days) Attachment 130288 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11829002 Comment on attachment 130288 [details] Theoretically unbreak wince (assuming anyone even compiles wince these days) Attachment 130288 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11830005 Created attachment 130304 [details]
Attempt to fix Gtk, Qt, Wx and WinCE
Comment on attachment 130304 [details] Attempt to fix Gtk, Qt, Wx and WinCE Attachment 130304 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11832191 Comment on attachment 130304 [details] Attempt to fix Gtk, Qt, Wx and WinCE Attachment 130304 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11838200 Mac (clean) builds definitely work for me locally. I've not added any files, so I wonder if this may be a incremental-build-only issue for the mac-ews. Win is showing a very similar issue to Mac. I wonder if it's getting confused about the same headers being in two places. :( I doubt I can remove the JavaScriptCore/Foo.h versions of wtf/Foo.h headers, as I suspect there may be Apple-internal things which depend on those. I will pick up this issue tomorrow. Hopefully will have clean builds from Gtk and EFL by then. Looking at the diff between the latest patch, and the one which supposedly passed the mac-ews: https://bugs.webkit.org/attachment.cgi?oldid=130288&action=interdiff&newid=130304&headers=1 There are no changes there which could have affected Mac. Leading me to believe this is an incremental-build issue. Comment on attachment 130304 [details] Attempt to fix Gtk, Qt, Wx and WinCE Attachment 130304 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11839356 Comment on attachment 130304 [details] Attempt to fix Gtk, Qt, Wx and WinCE View in context: https://bugs.webkit.org/attachment.cgi?id=130304&action=review I tried compiling the relvant classes and it seams to work. (Had no time to do a full WinCE build today, which takes a few hours :-() If it will break WinCE bot, I'm going to fix it. So do not let this change block by WinCE. > Source/WebCore/ForwardingHeaders/wtf/unicode/wince/UnicodeWince.h:-4 > -#ifndef WebCore_FWD_UnicodeWince_h > -#define WebCore_FWD_UnicodeWince_h > -#include <JavaScriptCore/UnicodeWince.h> > -#endif strange that this file was ever commited (https://bugs.webkit.org/show_bug.cgi?id=37224#c2) Created attachment 130452 [details]
Patch
After discussion with Mark Rowe last night, we decided to roll out the first part of this change which was done in bug 79960. This new patch includes both that earlier change as well as the previous patch from this bug. Additionally, this new patch now removes the WebKit ForwardingHeaders/wtf directory, as well as removes the WTF headers from JavaScriptCore private headers. I believe that the presence of these same headers in two different paths was causing the incremental build error. Created attachment 130455 [details]
fix Gtk build
Created attachment 130474 [details]
reupload in hopes the EWS will be kind to me
So I know why the EWS bubbles are failling to show, but I'd rather not bother to hack on the EWS system just to get this landed, so just re-uploading and prayin. Looks liek Mac and Win pass! Comment on attachment 130474 [details]
reupload in hopes the EWS will be kind to me
Marking as r- as clean builds are broken with this, as well as building most of the tools.
Created attachment 130521 [details]
Previous patch plus fixes for clean Mac build
Created attachment 130525 [details]
Updated with Mark's Mac build fixes
I've updated the patch with Mark's suggested changes and validated that "make" as well as build-dumprendertree and build-webkittestrunner ran cleanly on my machine. I've looked at Windows. I believe that no changes are necessary for Windows, because I "cheated" and re-used the /include/private/JavaScriptCore directory as base for wtf/*. It's possible that Windows will want to move to a /usr/local/include/wtf style to match Mac. But for now, this seemed the simplest (since I don't have a Windows machine to test on). I believe this patch is ready for official review and landing. I'm running anohter clean mac build locally just to triple check as well. Created attachment 130526 [details]
Add usr/local/include before ForwardingHeaders per discussion with Mark
I've re-confirmed that a full "make" build passes locally. We've seen all of the EWSes green for previous versions of this patch (except gtk, which is too far behind to bother with). This is ready for r+ and cq+ whenever someone has time. Thanks! I'll mark it r+ and commit-queue+ in an hour or so once I've had a chance to double-check that it doesn't cause any build issues. Comment on attachment 130526 [details] Add usr/local/include before ForwardingHeaders per discussion with Mark Attachment 130526 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11834771 Fixing TestWebKitAPI on Win now. Created attachment 130532 [details]
Attempt to fix Win TestWebKitAPI build
Comment on attachment 130532 [details] Attempt to fix Win TestWebKitAPI build Attachment 130532 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11833794 I appear to have edited the vcproj incorrectly? It's complaining about not finding WebKIt2 on win. I don't really know how else to try with TestWebKitAPI. As far as I can tell, my change looks correct. I think I'll either have to land as-is, or better yet, get someone with a Windows build to post an updated version of the patch, or instruct me how to fix TestWebKitAPI. It looks like QTMovieWin failed to build and so WebCore, WebKit and WebKit2 skipped building. I'm not sure why QTTrack.h has an #include <Unicode.h> in it. The fact that your change is causing it to fail to be found suggests that it may have previously been pulling in WTF's Unicode.h. That's almost certainly not needed in that file so the right thing to try here may be to remove that #include from QTTrack.h and see whether that gets QTMovieWin building. Created attachment 130544 [details]
attempt to fix Win
Created attachment 130546 [details]
Previous patch failed to include QTTrack changes
Comment on attachment 130546 [details] Previous patch failed to include QTTrack changes Attachment 130546 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11836775 Created attachment 130552 [details]
Fix windows harder
We're green on all ewses, and good to go. Comment on attachment 130552 [details]
Fix windows harder
Ok, what the heck.
Committed r110033: <http://trac.webkit.org/changeset/110033> |