Bug 80363 – Make WTF public headers use fully-qualified include paths and remove ForwardingHeaders/wtf

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make WTF public headers use fully-qualified include paths and remove WebCore/ForwardingHeaders/wtf

[Eric Seidel (no email)]

Created attachment 130265 [details]
Patch

[Eric Seidel (no email)]

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.

[Build Bot]

Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11832104

[Early Warning System Bot]

Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11836128

[Build Bot]

Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11838135

[Eric Seidel (no email)]

Created attachment 130285 [details]
same patch, but now should apply

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 130288 [details]
Theoretically unbreak wince (assuming anyone even compiles wince these days)

[Eric Seidel (no email)]

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.

[Early Warning System Bot]

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

[Build Bot]

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

[Eric Seidel (no email)]

Created attachment 130304 [details]
Attempt to fix Gtk, Qt, Wx and WinCE

[Build Bot]

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

[Build Bot]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I will pick up this issue tomorrow.  Hopefully will have clean builds from Gtk and EFL by then.

[Eric Seidel (no email)]

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.

[Gustavo Noronha (kov)]

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

[Patrick R. Gansterer]

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)

[Eric Seidel (no email)]

Created attachment 130452 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 130455 [details]
fix Gtk build

[Eric Seidel (no email)]

Created attachment 130474 [details]
reupload in hopes the EWS will be kind to me

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Looks liek Mac and Win pass!

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

Created attachment 130521 [details]
Previous patch plus fixes for clean Mac build

[Eric Seidel (no email)]

Created attachment 130525 [details]
Updated with Mark's Mac build fixes

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 130526 [details]
Add usr/local/include before ForwardingHeaders per discussion with Mark

[Eric Seidel (no email)]

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!

[Mark Rowe (bdash)]

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.

[Build Bot]

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

[Eric Seidel (no email)]

Fixing TestWebKitAPI on Win now.

[Eric Seidel (no email)]

Created attachment 130532 [details]
Attempt to fix Win TestWebKitAPI build

[Build Bot]

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

[Eric Seidel (no email)]

I appear to have edited the vcproj incorrectly?  It's complaining about not finding WebKIt2 on win.

[Eric Seidel (no email)]

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.

[Mark Rowe (bdash)]

It looks like QTMovieWin failed to build and so WebCore, WebKit and WebKit2 skipped building.

[Mark Rowe (bdash)]

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.

[Eric Seidel (no email)]

Created attachment 130544 [details]
attempt to fix Win

[Eric Seidel (no email)]

Created attachment 130546 [details]
Previous patch failed to include QTTrack changes

[Build Bot]

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

[Eric Seidel (no email)]

Created attachment 130552 [details]
Fix windows harder

[Eric Seidel (no email)]

We're green on all ewses, and good to go.

[Mark Rowe (bdash)]

Comment on attachment 130552 [details]
Fix windows harder

Ok, what the heck.

[Eric Seidel (no email)]

Committed r110033: <http://trac.webkit.org/changeset/110033>