Bug 80363 - Make WTF public headers use fully-qualified include paths and remove ForwardingHeaders/wtf
Summary: Make WTF public headers use fully-qualified include paths and remove Forwardi...
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:
Blocks: 75673
  Show dependency treegraph
 
Reported: 2012-03-05 18:50 PST by Eric Seidel (no email)
Modified: 2012-03-07 00:51 PST (History)
8 users (show)

See Also:


Attachments
Patch (109.19 KB, patch)
2012-03-05 18:54 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
same patch, but now should apply (109.16 KB, patch)
2012-03-05 22:02 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Theoretically unbreak wince (assuming anyone even compiles wince these days) (109.10 KB, patch)
2012-03-05 22:14 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Attempt to fix Gtk, Qt, Wx and WinCE (109.14 KB, patch)
2012-03-05 23:22 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (249.24 KB, patch)
2012-03-06 15:27 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
fix Gtk build (249.24 KB, patch)
2012-03-06 15:43 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
reupload in hopes the EWS will be kind to me (249.24 KB, patch)
2012-03-06 16:51 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Previous patch plus fixes for clean Mac build (248.53 KB, patch)
2012-03-06 19:24 PST, Mark Rowe (bdash)
no flags Details | Formatted Diff | Diff
Updated with Mark's Mac build fixes (269.04 KB, patch)
2012-03-06 19:54 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add usr/local/include before ForwardingHeaders per discussion with Mark (268.93 KB, patch)
2012-03-06 20:04 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Attempt to fix Win TestWebKitAPI build (270.19 KB, patch)
2012-03-06 21:06 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
attempt to fix Win (270.65 KB, patch)
2012-03-06 22:22 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Previous patch failed to include QTTrack changes (270.65 KB, patch)
2012-03-06 22:37 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix windows harder (273.63 KB, patch)
2012-03-06 23:22 PST, Eric Seidel (no email)
mrowe: review+
mrowe: commit-queue+
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-03-05 18:50:00 PST
Make WTF public headers use fully-qualified include paths and remove WebCore/ForwardingHeaders/wtf
Comment 1 Eric Seidel (no email) 2012-03-05 18:54:44 PST
Created attachment 130265 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-03-05 18:59:18 PST
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 3 Build Bot 2012-03-05 19:47:51 PST
Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11832104
Comment 4 Early Warning System Bot 2012-03-05 20:45:53 PST
Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11836128
Comment 5 Build Bot 2012-03-05 21:23:36 PST
Comment on attachment 130265 [details]
Patch

Attachment 130265 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11838135
Comment 6 Eric Seidel (no email) 2012-03-05 22:02:23 PST
Created attachment 130285 [details]
same patch, but now should apply
Comment 7 Eric Seidel (no email) 2012-03-05 22:10:41 PST
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.
Comment 8 Eric Seidel (no email) 2012-03-05 22:14:50 PST
Created attachment 130288 [details]
Theoretically unbreak wince (assuming anyone even compiles wince these days)
Comment 9 Eric Seidel (no email) 2012-03-05 22:15:34 PST
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 10 Early Warning System Bot 2012-03-05 22:50:04 PST
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 11 Build Bot 2012-03-05 22:55:42 PST
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
Comment 12 Eric Seidel (no email) 2012-03-05 23:22:48 PST
Created attachment 130304 [details]
Attempt to fix Gtk, Qt, Wx and WinCE
Comment 13 Build Bot 2012-03-06 00:23:01 PST
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 14 Build Bot 2012-03-06 01:08:27 PST
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
Comment 15 Eric Seidel (no email) 2012-03-06 01:09:16 PST
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.
Comment 16 Eric Seidel (no email) 2012-03-06 01:12:45 PST
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.
Comment 17 Eric Seidel (no email) 2012-03-06 01:13:17 PST
I will pick up this issue tomorrow.  Hopefully will have clean builds from Gtk and EFL by then.
Comment 18 Eric Seidel (no email) 2012-03-06 01:18:04 PST
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 19 Gustavo Noronha (kov) 2012-03-06 06:26:27 PST
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 20 Patrick R. Gansterer 2012-03-06 09:54:40 PST
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)
Comment 21 Eric Seidel (no email) 2012-03-06 15:27:57 PST
Created attachment 130452 [details]
Patch
Comment 22 Eric Seidel (no email) 2012-03-06 15:30:16 PST
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.
Comment 23 Eric Seidel (no email) 2012-03-06 15:43:40 PST
Created attachment 130455 [details]
fix Gtk build
Comment 24 Eric Seidel (no email) 2012-03-06 16:51:22 PST
Created attachment 130474 [details]
reupload in hopes the EWS will be kind to me
Comment 25 Eric Seidel (no email) 2012-03-06 16:52:36 PST
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.
Comment 26 Eric Seidel (no email) 2012-03-06 17:43:54 PST
Looks liek Mac and Win pass!
Comment 27 Mark Rowe (bdash) 2012-03-06 19:21:42 PST
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.
Comment 28 Mark Rowe (bdash) 2012-03-06 19:24:14 PST
Created attachment 130521 [details]
Previous patch plus fixes for clean Mac build
Comment 29 Eric Seidel (no email) 2012-03-06 19:54:38 PST
Created attachment 130525 [details]
Updated with Mark's Mac build fixes
Comment 30 Eric Seidel (no email) 2012-03-06 19:57:26 PST
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.
Comment 31 Eric Seidel (no email) 2012-03-06 20:04:02 PST
Created attachment 130526 [details]
Add usr/local/include before ForwardingHeaders per discussion with Mark
Comment 32 Eric Seidel (no email) 2012-03-06 20:23:05 PST
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!
Comment 33 Mark Rowe (bdash) 2012-03-06 20:25:34 PST
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 34 Build Bot 2012-03-06 20:26:24 PST
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
Comment 35 Eric Seidel (no email) 2012-03-06 20:59:15 PST
Fixing TestWebKitAPI on Win now.
Comment 36 Eric Seidel (no email) 2012-03-06 21:06:35 PST
Created attachment 130532 [details]
Attempt to fix Win TestWebKitAPI build
Comment 37 Build Bot 2012-03-06 21:34:40 PST
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
Comment 38 Eric Seidel (no email) 2012-03-06 21:36:47 PST
I appear to have edited the vcproj incorrectly?  It's complaining about not finding WebKIt2 on win.
Comment 39 Eric Seidel (no email) 2012-03-06 21:42:44 PST
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.
Comment 40 Mark Rowe (bdash) 2012-03-06 21:43:13 PST
It looks like QTMovieWin failed to build and so WebCore, WebKit and WebKit2 skipped building.
Comment 41 Mark Rowe (bdash) 2012-03-06 21:50:49 PST
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.
Comment 42 Eric Seidel (no email) 2012-03-06 22:22:54 PST
Created attachment 130544 [details]
attempt to fix Win
Comment 43 Eric Seidel (no email) 2012-03-06 22:37:00 PST
Created attachment 130546 [details]
Previous patch failed to include QTTrack changes
Comment 44 Build Bot 2012-03-06 22:59:15 PST
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
Comment 45 Eric Seidel (no email) 2012-03-06 23:22:06 PST
Created attachment 130552 [details]
Fix windows harder
Comment 46 Eric Seidel (no email) 2012-03-07 00:12:13 PST
We're green on all ewses, and good to go.
Comment 47 Mark Rowe (bdash) 2012-03-07 00:15:38 PST
Comment on attachment 130552 [details]
Fix windows harder

Ok, what the heck.
Comment 48 Eric Seidel (no email) 2012-03-07 00:51:11 PST
Committed r110033: <http://trac.webkit.org/changeset/110033>