Bug 24653 - WebKit should be buildable without prefix header injection
Summary: WebKit should be buildable without prefix header injection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Mark Mentovai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-17 14:09 PDT by Mark Mentovai
Modified: 2009-03-24 11:26 PDT (History)
3 users (show)

See Also:


Attachments
Adds missing #includes and forward declarations as needed (11.92 KB, patch)
2009-03-17 14:15 PDT, Mark Mentovai
mrowe: review-
Details | Formatted Diff | Diff
Sorted as requested (11.88 KB, patch)
2009-03-17 14:58 PDT, Mark Mentovai
mrowe: review+
Details | Formatted Diff | Diff
Without NSUInteger problems (11.87 KB, patch)
2009-03-17 15:39 PDT, Mark Mentovai
mrowe: review+
Details | Formatted Diff | Diff
v4 (11.79 KB, patch)
2009-03-17 16:17 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
v5 (11.79 KB, patch)
2009-03-17 16:23 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
v6 (12.21 KB, patch)
2009-03-20 12:49 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Mentovai 2009-03-17 14:09:44 PDT
For the Chromium Mac build, I'm working on a system to speed along our sluggish buildbot-driven continuous builds.  This system involves using distcc in pump mode to distribute the compilation to a number of systems, some of which are not even Macs.  (They're running Linux and have a full Apple cross-toolchain.)

distcc's pump mode is not compatible with prefix header injection ("gcc -include" or the GCC_PREFIX_HEADER Xcode setting.)

The WebCore Mac build is currently dependent on prefix header injection.  It precompiles WebCorePrefix.h.  It seems that Mac-specific parts of WebCore are the only parts that actually rely on a prefix header; all of the platform-independent bits build without prefix header injection.

The Chromium Mac build still builds some Mac files from WebCore.

In order to get everything to work properly with my new distcc setup, I need to make WebCore buildable without prefix header injection, even on the Mac.  To be clear, I am not eliminating the ability to build with a [precompiled] prefix header.  I'm merely adding the #includes and forward declarations that are needed to make the Mac-specific portions of WebCore used by Chromium build when the prefix header is not in use.

Further reading on distcc's pump mode: http://google-opensource.blogspot.com/2008/08/distccs-pump-mode-new-design-for.html .
Comment 1 Eric Seidel (no email) 2009-03-17 14:12:35 PDT
Hopefully Apple will chose to look at shipping distcc 3.0 in the next Xcode release anyway. :)  In which case the Apple WebKit folks will be incentivized to make pump-mode work too! :)
Comment 2 Mark Mentovai 2009-03-17 14:15:47 PDT
Created attachment 28698 [details]
Adds missing #includes and forward declarations as needed

Hopefully indeed.

Eric or Dimitri, if you would be so kind...
Comment 3 Eric Seidel (no email) 2009-03-17 14:17:28 PDT
Comment on attachment 28698 [details]
Adds missing #includes and forward declarations as needed

This is not something for Dimitri or I to review.  Mark Rowe, or someone who works with the Mac port at Apple should review this.
Comment 4 Mark Rowe (bdash) 2009-03-17 14:40:50 PDT
Comment on attachment 28698 [details]
Adds missing #includes and forward declarations as needed

The includes need to be consistently sorted in case-sensitive alphabetical order.  In some places this was done, but in others they were not sorted or were added in a separate paragraph from existing includes.  #import or #include should be used consistently within FoundationExtras.h, and the includes should not be added in between the comment describing HardRetain/HardRelease and their definitions.
Comment 5 Mark Mentovai 2009-03-17 14:58:11 PDT
Created attachment 28701 [details]
Sorted as requested
Comment 6 Mark Rowe (bdash) 2009-03-17 15:25:47 PDT
Comment on attachment 28701 [details]
Sorted as requested

> +typedef unsigned int NSFontTraitMask;

In 10.5 and beyond NSFontTraitMask is declared by AppKit as a NSUInteger.  I suspect that this will lead to problems, possibly a build breakage, in 64-bit where NSUInteger is a typedef of unsigned long rather than unsigned int.

r=me if you've verified that this doesn't break the 64-bit build.
Comment 7 Mark Mentovai 2009-03-17 15:39:24 PDT
Created attachment 28705 [details]
Without NSUInteger problems

Good catch.  This version just gets that typedef from the header to minimize conflicts.  NSFontManager.h needs to be #imported (no guards), so I've flipped the other #include in that file over to be consistent.
Comment 8 Mark Mentovai 2009-03-17 15:47:44 PDT
Comment on attachment 28705 [details]
Without NSUInteger problems

One more coming
Comment 9 Mark Rowe (bdash) 2009-03-17 15:48:33 PDT
Comment on attachment 28705 [details]
Without NSUInteger problems

Now that you're including NSFontManager.h, there's no need to forward declare NSPoint or NSRect.  NSFontManager.h includes NSGeometry.h, which provides the real declarations for NSPoint and NSRect.
Comment 10 Mark Mentovai 2009-03-17 16:17:47 PDT
Created attachment 28708 [details]
v4

OK, in addition to the removal of the NSPoint/NSRect forward declarations, this version is more careful to use #import for the guard-less AppKit headers, and fixes the order in one file I missed the sort on.

I think this should be the final version.  It works with both the Chromium build (with prefix header injection disabled) and the stock WebKit Mac build (with the precompiled prefix header).
Comment 11 Mark Mentovai 2009-03-17 16:23:20 PDT
Created attachment 28709 [details]
v5

Sorry for the noise.  This one moves a Foundation #include to #import for the same reason, Foundation is guard-less and expects to be #imported.
Comment 12 Mark Mentovai 2009-03-20 12:04:27 PDT
Mark, I'm done making extra patches here now, this is ready for final review.
Comment 13 Eric Seidel (no email) 2009-03-20 12:17:25 PDT
Comment on attachment 28709 [details]
v5

The include of objc/objc.h in PlatformString.h seems a bit strange.  PlatformString.h is included in almost every file in WebCore, so it seems like you're trying to use it as a prefix header here... unless there is some obj-c specific logic in that file?
Comment 14 Mark Rowe (bdash) 2009-03-20 12:24:22 PDT
(In reply to comment #13)
> (From update of attachment 28709 [details] [review])
> The include of objc/objc.h in PlatformString.h seems a bit strange. 
> PlatformString.h is included in almost every file in WebCore, so it seems like
> you're trying to use it as a prefix header here... unless there is some obj-c
> specific logic in that file?
> 

Presumably it needs this for the definition of "nil" which is used later on in the file.
Comment 15 Mark Mentovai 2009-03-20 12:27:05 PDT
That's correct, objc.h is needed for http://trac.webkit.org/browser/trunk/WebCore/platform/text/PlatformString.h#L282 .
Comment 16 Mark Mentovai 2009-03-20 12:49:58 PDT
Created attachment 28797 [details]
v6

Needed one more change, ColorMac.mm uses NSApp and was apparently getting it via some other #import until recently.
Comment 17 Mark Mentovai 2009-03-20 14:56:22 PDT
Comment on attachment 28797 [details]
v6

Does this need another r+ or are the earlier r+s enough to get this checked in?
Comment 18 Dimitri Glazkov (Google) 2009-03-24 10:06:38 PDT
Landed as http://trac.webkit.org/changeset/41939.
Comment 19 Darin Adler 2009-03-24 10:58:39 PDT
It’s highly likely this will break over and over again. I’m not sure this is the right solution long term.
Comment 20 Mark Mentovai 2009-03-24 11:14:00 PDT
I'm aware, the long-term solution is actually for Chromium Mac to continue reducing dependencies on WebKit Mac platform code, except where it really makes sense to share.  We'll* just need to keep up with this for the time being, since some of the Chromium builds depend on no precompilation (making the prefix header a loss) and no prefix headers at all.

* as the de facto Chromium Mac build czar, I know what "we" means.
Comment 21 Eric Seidel (no email) 2009-03-24 11:26:50 PDT
(In reply to comment #20)
> We'll* just need to keep up with this for the time being,
> since some of the Chromium builds depend on no precompilation (making the
> prefix header a loss) and no prefix headers at all.

We have a Chromium Mac buildbot in mountain view wired up to build.webkit.org.  Currently it's turned off because build-webkit --chromium is broken.  Perhaps we should have it build some other version of webkit with no precomp?  If there would be an easy way to set up such a build, it's certainly possible to add a webkit.org builder for it.