Bug 135662 - cmake is coming along nicely on mac
Summary: cmake is coming along nicely on mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-06 11:23 PDT by Alex Christensen
Modified: 2014-08-08 11:14 PDT (History)
14 users (show)

See Also:


Attachments
Patch (19.37 KB, patch)
2014-08-06 11:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (19.38 KB, patch)
2014-08-06 11:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (22.96 KB, patch)
2014-08-06 13:01 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (22.18 KB, patch)
2014-08-06 13:45 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.83 KB, patch)
2014-08-07 11:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.62 KB, patch)
2014-08-07 12:22 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.47 KB, patch)
2014-08-07 12:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.41 KB, patch)
2014-08-07 14:48 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.75 KB, patch)
2014-08-08 09:32 PDT, Alex Christensen
laszlo.gombos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2014-08-06 11:23:07 PDT
WebCore builds most of the way.  There are some objc quirks still, but this is a lot of progress.
Comment 1 Alex Christensen 2014-08-06 11:31:56 PDT
Created attachment 236120 [details]
Patch
Comment 2 Alex Christensen 2014-08-06 11:34:00 PDT
Created attachment 236121 [details]
Patch
Comment 3 Laszlo Gombos 2014-08-06 11:54:29 PDT
Comment on attachment 236121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236121&action=review

> ChangeLog:13
> +        * Source/cmake/WebKitFeatures.cmake:

Sadly we should also list all the new flags to cmakeconfig.h.cmake for consistency.
Comment 4 Alex Christensen 2014-08-06 13:01:28 PDT
Created attachment 236127 [details]
Patch
Comment 5 Alex Christensen 2014-08-06 13:02:24 PDT
(In reply to comment #3)
> Sadly we should also list all the new flags to cmakeconfig.h.cmake for consistency.
Thanks.  That explains a few quirks I was running into.  Those two files are not as consistent as you might think, but I made my same changes to both.
Comment 6 Csaba Osztrogonác 2014-08-06 13:36:32 PDT
Comment on attachment 236127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236127&action=review

Just out of curiosity: Is there a plan to migrate the Apple Mac port to cmake?

> Source/WebCore/CMakeLists.txt:3595
> -    target_include_directories(ANGLESupport PRIVATE "${THIRDPARTY_DIR}/ANGLE/include")
> +    include_directories(ANGLESupport PRIVATE "${THIRDPARTY_DIR}/ANGLE/include")

There was a reason why target_include_directories is used and 
not include_directories. Please check was 171475 for details.
And that's why minimum cmake version was bumped to 2.8.11
Comment 7 Alex Christensen 2014-08-06 13:41:30 PDT
(In reply to comment #6)
> (From update of attachment 236127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236127&action=review
> 
> Just out of curiosity: Is there a plan to migrate the Apple Mac port to cmake?
I'm working on a proof-of-concept right now.  I'm definitely not saying it will be immediately accepted by everyone at Apple.
> 
> > Source/WebCore/CMakeLists.txt:3595
> > -    target_include_directories(ANGLESupport PRIVATE "${THIRDPARTY_DIR}/ANGLE/include")
> > +    include_directories(ANGLESupport PRIVATE "${THIRDPARTY_DIR}/ANGLE/include")
> 
> There was a reason why target_include_directories is used and 
> not include_directories. Please check was 171475 for details.
> And that's why minimum cmake version was bumped to 2.8.11
I'll remove this change and we'll worry about this in a later patch.
Comment 8 Alex Christensen 2014-08-06 13:45:58 PDT
Created attachment 236131 [details]
Patch
Comment 9 Alex Christensen 2014-08-07 11:51:25 PDT
Created attachment 236202 [details]
Patch
Comment 10 Alex Christensen 2014-08-07 12:22:33 PDT
Created attachment 236207 [details]
Patch
Comment 11 Alex Christensen 2014-08-07 12:29:17 PDT
Created attachment 236208 [details]
Patch
Comment 12 Mark Rowe (bdash) 2014-08-07 12:48:36 PDT
Comment on attachment 236208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236208&action=review

> Source/WebCore/config.h:64
> +// Using CMake with Unix makefiles does not use precompiled headers.

Presumably the problem here is the lack of prefix headers. Whether they're precompiled or not doesn't seem relevant.
Comment 13 Alex Christensen 2014-08-07 14:03:04 PDT
(In reply to comment #12)
> (From update of attachment 236208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236208&action=review
> 
> > Source/WebCore/config.h:64
> > +// Using CMake with Unix makefiles does not use precompiled headers.
> 
> Presumably the problem here is the lack of prefix headers. Whether they're precompiled or not doesn't seem relevant.
Yes, that's exactly it.

I can't figure out why GTK and EFL don't like this patch.  I don't think the GTK failure is my fault, and I think the VTT stuff on EFL shouldn't have been changed.
Comment 14 Laszlo Gombos 2014-08-07 14:17:28 PDT
Comment on attachment 236208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236208&action=review

> Source/WebCore/ChangeLog:12
> +        Moved ImageSource.cpp and image-decoders to platform CMake files because they are not used on mac.

Hmm, but they used everywhere else but the mac. Can we somehow make them conditional instead (either with a CMake variable or in the cpp files with a PLATFORM()/OS() check ) ?
Comment 15 Alex Christensen 2014-08-07 14:22:08 PDT
(In reply to comment #14)
> (From update of attachment 236208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236208&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Moved ImageSource.cpp and image-decoders to platform CMake files because they are not used on mac.
> 
> Hmm, but they used everywhere else but the mac. Can we somehow make them conditional instead (either with a CMake variable or in the cpp files with a PLATFORM()/OS() check ) ?

They're not used in AppleWin either.  Just WinCairo, EFL, and GTK.  I think they should go in those platform files.
Comment 16 Alex Christensen 2014-08-07 14:35:23 PDT
I don't think I trust the EWS bots with this patch.
Comment 17 Laszlo Gombos 2014-08-07 14:41:53 PDT
(In reply to comment #16)
> I don't think I trust the EWS bots with this patch.

Seems the patch does apply on the win bot either - https://webkit-queues.appspot.com/results/5588216753160192

Perhaps because http://trac.webkit.org/changeset/172259 just landed ?
Comment 18 Alex Christensen 2014-08-07 14:48:11 PDT
Created attachment 236229 [details]
Patch
Comment 19 Alex Christensen 2014-08-08 09:32:52 PDT
Created attachment 236282 [details]
Patch
Comment 20 Alex Christensen 2014-08-08 10:51:12 PDT
I'm pretty sure the EFL failure is not the fault of this patch.
Comment 21 Laszlo Gombos 2014-08-08 11:08:01 PDT
Comment on attachment 236282 [details]
Patch

LGTM
Comment 22 Alex Christensen 2014-08-08 11:14:26 PDT
http://trac.webkit.org/changeset/172346