Bug 231749 – Realize Mac CMake build of WebCore and WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Ross Kirsling]

Realize full Mac CMake build

[Ross Kirsling]

Created attachment 441239 [details]
Patch

[Ross Kirsling]

It lives! (Patch is based off of the one in bug 225070.)

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Ross Kirsling]

Created attachment 441272 [details]
Patch

[Ross Kirsling]

This certainly can't land with the #if 1 in Tools/MiniBrowser/mac/WK1BrowserWindowController.h, but the error there is weird, as its #imports of WKL things are conflicting with WebKit.framework headers:

> In file included from .../Tools/MiniBrowser/mac/WK1BrowserWindowController.m:34:
> In file included from .../WebKitBuild/Release/WebKitLegacy/Headers/WebKitLegacy/WebPreferencesPrivate.h:1:
> In file included from .../Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:29:
> In file included from .../WebKitBuild/Release/WebKitLegacy/Headers/WebKitLegacy/WebPreferences.h:1:
> .../Source/WebKitLegacy/mac/WebView/WebPreferences.h:167:38: error: property has a previous declaration
> @property (nonatomic, strong) NSURL *userStyleSheetLocation;
>                                      ^
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/WebKit.framework/Headers/WebPreferences.h:167:38: note: property declared here
> @property (nonatomic, strong) NSURL *userStyleSheetLocation;
>                                     ^

[Alex Christensen]

Comment on attachment 441272 [details]
Patch

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

> Source/JavaScriptCore/PlatformMac.cmake:-40
> -set(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-compatibility_version 1 -current_version ${WEBKIT_MAC_VERSION} -force_load ${CMAKE_BINARY_DIR}/lib/libWTF.a -force_load ${CMAKE_BINARY_DIR}/lib/libbmalloc.a")

As strange as it is, this needs to stay.

> Source/WebCore/PlatformMac.cmake:-941
> -set(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-compatibility_version 1 -current_version ${WEBKIT_MAC_VERSION} -force_load ${CMAKE_BINARY_DIR}/lib/libPAL.a")

ditto

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:73
> +static NSMenu *gCurrentPopUpMenu = nil;

This could be a WeakObjCPtr

[Ross Kirsling]

Created attachment 441276 [details]
Patch

[Ross Kirsling]

Okay, so judging from EWS, the Tools changes I was working with might have been based on false assumptions, so I'm stashing those for now and we can focus on just having WebCore and WebKit fully buildable for this patch.

[Don Olmstead]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 441272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441272&action=review
> 
> > Source/JavaScriptCore/PlatformMac.cmake:-40
> > -set(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-compatibility_version 1 -current_version ${WEBKIT_MAC_VERSION} -force_load ${CMAKE_BINARY_DIR}/lib/libWTF.a -force_load ${CMAKE_BINARY_DIR}/lib/libbmalloc.a")
> 
> As strange as it is, this needs to stay.
> 
> > Source/WebCore/PlatformMac.cmake:-941
> > -set(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-compatibility_version 1 -current_version ${WEBKIT_MAC_VERSION} -force_load ${CMAKE_BINARY_DIR}/lib/libPAL.a")
> 
> ditto

The OBJECT libraries are a cross platform way of doing GCC's `--whole-archive` flag, which `-force_load` is roughly equivalent to. When there's an OBJECT library it just builds out all the .o files. Then in this case the .o files for bmalloc and WTF get linked into JavaScriptCore and the .o files for PAL get linked into WebCore. If this behavior wasn't working then there would be linker errors.

[Don Olmstead]

Comment on attachment 441276 [details]
Patch

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

> Source/cmake/WebKitMacros.cmake:245
> +        if (APPLE)

Ross can you add a FIXME here for https://bugs.webkit.org/show_bug.cgi?id=231774

This was done to prevent WebCore from ending up in the `_LIBRARIES` when using the `-umbrella` flag which was causing a linker error but I have some ideas on how to better handle this which I want to try out in that linked bug.

[Ross Kirsling]

Created attachment 441294 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 441294 [details]
Patch

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

> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:30
> +#include "ActiveDOMObject.h"

ApplePaySetupWebCore.h is a private header, so this is still not OK.

[Don Olmstead]

Comment on attachment 441294 [details]
Patch

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

>> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:30
>> +#include "ActiveDOMObject.h"
> 
> ApplePaySetupWebCore.h is a private header, so this is still not OK.

Its my understanding that WebCore doesn't have any public headers only private so this would be a valid change.

Did something change here?

[Alexey Proskuryakov]

"Private" means that it's not part of public Xcode SDK, but unlike "Project" headers, these are part of internal SDK.

I do see that there is another non-framework-style include next to this one, and I don't know why exactly this works, but it's still a problem with CMake build as proposed if it requires to use the wrong include form.

[Ross Kirsling]

This is in fact the *only* file under WebCore/Modules with a `<WebCore/...>` include, so it certainly looks like a mistake...

[Ross Kirsling]

(In reply to Ross Kirsling from comment #15)
> This is in fact the *only* file under WebCore/Modules with a `<WebCore/...>`
> include, so it certainly looks like a mistake...

In fact, it and testing/cocoa/WebViewVisualIdentificationOverlay.h are the only files in the whole of WebCore which are doing this in the Mac build.

My understanding is that a <WebCore/...> include is only used to consume a framework from the outside, so fixing this syntax should be desirable even without talking about CMake.

[Ross Kirsling]

Created attachment 441436 [details]
Patch

[Ross Kirsling]

I have split out source changes from this patch; we can continue that discussion in a separate non-CMake-related ticket.

[Ross Kirsling]

Created attachment 441439 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 441439 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        * SourcesCocoa.txt:
> +        * WebKit.xcodeproj/project.pbxproj:

Could you please add an explanation for this change? As it's the only non-CMake change here, I'm just curious.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #20)
> Comment on attachment 441439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441439&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        * SourcesCocoa.txt:
> > +        * WebKit.xcodeproj/project.pbxproj:
> 
> Could you please add an explanation for this change? As it's the only
> non-CMake change here, I'm just curious.

Platform/IPC/cocoa/ sources are listed in SourcesCocoa.txt except for this one which was added as "Sources" in Xcode instead. A hackier temporary solution would be to list this one file in PlatformMac.cmake instead, to avoid editing the xcodeproj -- I suppose I could do that for this patch, because there are other files which ought to be moved similarly in a subsequent patch.

[Ross Kirsling]

Created attachment 441456 [details]
Patch for landing

[EWS]

Committed r284298 (243096@main): <https://commits.webkit.org/243096@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441456 [details].