Bug 135528 - progress towards cmake on mac
Summary: progress towards cmake 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-01 16:02 PDT by Alex Christensen
Modified: 2014-08-04 17:31 PDT (History)
11 users (show)

See Also:


Attachments
Patch (6.58 KB, patch)
2014-08-01 16:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2014-08-01 16:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2014-08-04 16:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (28.37 KB, patch)
2014-08-04 16:57 PDT, Alex Christensen
gyuyoung.kim: 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-01 16:02:03 PDT
Here's some more work to review.  I'm not sure if copying all the API headers to the derived sources directory is similar to anything else that is done or if there's a better place for them, but they need to be copied to somewhere to be accessed by #include <JavaScriptCore/APIHeader.h>
Comment 1 Alex Christensen 2014-08-01 16:07:19 PDT
Created attachment 235919 [details]
Patch
Comment 2 Martin Robinson 2014-08-01 16:08:35 PDT
Comment on attachment 235919 [details]
Patch

Where is APPLE defined or is it a CMake builtin?
Comment 3 Martin Robinson 2014-08-01 16:09:51 PDT
Comment on attachment 235919 [details]
Patch

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

> Source/JavaScriptCore/CMakeLists.txt:596
> +if (APPLE)
> +    list(APPEND JavaScriptCore_SOURCES
> +        API/JSAPIWrapperObject.mm
> +        API/JSContext.mm
> +        API/JSManagedValue.mm
> +        API/JSStringRefCF.cpp
> +        API/JSValue.mm
> +        API/JSVirtualMachine.mm
> +        API/JSWrapperMap.mm
> +        API/ObjCCallbackFunction.mm
>      )

If these are specific to the Mac port they should probably be moved to the PlatformMac.cmake file.

> Source/WTF/wtf/CMakeLists.txt:269
> +if (APPLE)
> +    list(APPEND WTF_SOURCES
> +        text/cf/AtomicStringCF.cpp
> +        text/cf/StringCF.cpp
> +        text/cf/StringImplCF.cpp
> +        text/cf/StringViewCF.cpp
> +        text/mac/StringMac.mm
> +        text/mac/StringImplMac.mm
> +        text/mac/StringViewObjC.mm
> +        mac/MainThreadMac.mm
> +        mac/DeprecatedSymbolsUsedBySafari.mm
> +    )
> +endif ()

Ditto. Platform-specific source files should go in the Platform*.cmake files.
Comment 4 Mark Rowe (bdash) 2014-08-01 16:15:57 PDT
(In reply to comment #0)
> Here's some more work to review.  I'm not sure if copying all the API headers to the derived sources directory is similar to anything else that is done or if there's a better place for them, but they need to be copied to somewhere to be accessed by #include <JavaScriptCore/APIHeader.h>

Shouldn't the API headers be copied in to the framework wrapper and accessed from there?
Comment 5 Alex Christensen 2014-08-01 16:52:57 PDT
Created attachment 235924 [details]
Patch
Comment 6 Alex Christensen 2014-08-04 16:05:03 PDT
Created attachment 235988 [details]
Patch
Comment 7 Alex Christensen 2014-08-04 16:09:26 PDT
jsc compiles, links, and runs on Mac with this.  No FTL yet :(
I started into WebCore, but that's WIP.
Comment 8 Alex Christensen 2014-08-04 16:57:40 PDT
Created attachment 235990 [details]
Patch
Comment 9 Gyuyoung Kim 2014-08-04 17:18:50 PDT
Comment on attachment 235990 [details]
Patch

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

As an initial patch for supporting CMake on mac, I think there is no critical issue except for minor nits.

BTW, don't you need to add PlatformMac.cmake for Source/WebKit2 dir ?

> Source/JavaScriptCore/CMakeLists.txt:17
> +    "${JAVASCRIPTCORE_DIR}/inspector/remote"

Wrong alphabet order.

> Source/JavaScriptCore/CMakeLists.txt:32
> +    "${DERIVED_SOURCES_DIR}/ForwardingHeaders"

ditto ?

> Source/JavaScriptCore/CMakeLists.txt:584
> +        disassembler/UDis86Disassembler.cpp

Isn't upper case placed to top side ?

> Source/JavaScriptCore/PlatformMac.cmake:6
> +    ${COCOA_LIBRARY}

ditto.

> Source/cmake/OptionsMac.cmake:32
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS3_TEXT_LINE_BREAK OFF)

ditto.

> Source/cmake/OptionsMac.cmake:41
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_DEVICE_ORIENTATION_iphoneos ON)

As far as I know, we have defined option values in Source/cmake/WebKitFeatures.cmake, then use it for each port. Can these mac port specific values be enabled when you build on mac ?
Comment 10 Alex Christensen 2014-08-04 17:31:05 PDT
Fixed some alphabetization and committed to http://trac.webkit.org/changeset/172014