Bug 198446 – [PlayStation] Build a shared JavaScriptCore

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

[Don Olmstead]

JSC should be built as a SHARED library.

[Don Olmstead]

Created attachment 371191 [details]
WIP Patch

[Michael Catanzaro]

Comment on attachment 371191 [details]
WIP Patch

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

So this crack works?

> Source/JavaScriptCore/PlatformPlayStation.cmake:41
> +# Both bmalloc and WTF are built as object libraries. Link them into JavaScriptCore
> +# privately so the object files do not propagate.

Otherwise we would wind up with two separate copies of both bmalloc and WTF. Right? Might be worth explaining in the comment?

[Don Olmstead]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 371191 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371191&action=review
> 
> So this crack works?
> 
> > Source/JavaScriptCore/PlatformPlayStation.cmake:41
> > +# Both bmalloc and WTF are built as object libraries. Link them into JavaScriptCore
> > +# privately so the object files do not propagate.
> 
> Otherwise we would wind up with two separate copies of both bmalloc and WTF.
> Right? Might be worth explaining in the comment?

This hack does work yes. Chris is trying something similar with GTK. Posting this now to just give you an idea on what is going on. We'll land this eventually.

[Don Olmstead]

Created attachment 389741 [details]
Patch

[Don Olmstead]

Created attachment 389742 [details]
Patch

[Stephan Szabo]

Comment on attachment 389742 [details]
Patch

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

> Source/JavaScriptCore/PlatformPlayStation.cmake:60
> +    STATICALLY_LINKED_WITH_WTF

It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.

[Don Olmstead]

Comment on attachment 389742 [details]
Patch

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

>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>> +    STATICALLY_LINKED_WITH_WTF
> 
> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.

This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.

Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.

[Fujii Hironori]

Comment on attachment 389742 [details]
Patch

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

>>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>>> +    STATICALLY_LINKED_WITH_WTF
>> 
>> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.
> 
> This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.
> 
> Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.

Weird. WTF_PRIVATE_DEFINITIONS shouldn't be propagated because it is PRIVATE.

> Tools/TestWebKitAPI/PlatformPlayStation.cmake:19
> +)

You don't need to use TARGET_OBJECTS.
See the second example of the document.
https://cmake.org/cmake/help/v3.16/manual/cmake-buildsystem.7.html#object-libraries

Actually, WinCairo port is using OBJECT library for WebCore, but not using TARGET_OBJECTS for it.

[Don Olmstead]

Comment on attachment 389742 [details]
Patch

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

>>>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>>>> +    STATICALLY_LINKED_WITH_WTF
>>> 
>>> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.
>> 
>> This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.
>> 
>> Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.
> 
> Weird. WTF_PRIVATE_DEFINITIONS shouldn't be propagated because it is PRIVATE.

I'll add it its not like it hurts anything. CMake can be weird.

>> Tools/TestWebKitAPI/PlatformPlayStation.cmake:19
>> +)
> 
> You don't need to use TARGET_OBJECTS.
> See the second example of the document.
> https://cmake.org/cmake/help/v3.16/manual/cmake-buildsystem.7.html#object-libraries
> 
> Actually, WinCairo port is using OBJECT library for WebCore, but not using TARGET_OBJECTS for it.

There is not currently a WebKit::WebCore interface target. WebCore is used directly. That works fine which as WinCairo shows. In this case there are WebKit::WTF and WebKit::bmalloc interface targets that depend on the OBJECT library and the TARGET_OBJECTS does not propagate to targets that depend on the interface target. This can be fixed by using $<TARGET_OBJECTS> directly when defining ${target}_INTERFACE_LIBRARIES if an OBJECT library is being used.

However the CMake is not in a great state when it comes to properly using PUBLIC and PRIVATE. There's also a lot of difference between the ports for how they present different libraries within the project. Windows does everything shared except WebCore and PAL, Mac does JavaScriptCore WebCore and WebKit as shared, WPE only has WebKit as shared. This really needs to be tamed before any of the logic for OBJECT libraries get wrapped into WEBKIT_FRAMEWORK. So setting the $<TARGET_OBJECTS> in the JavaScriptCore Platform file makes it so we don't have any problems with a SHARED JavaScriptCore with multiply defined symbols. I agree its not ideal but this is a workaround for the current state of the build system.

[Don Olmstead]

Created attachment 389877 [details]
Patch

Address feedback

[Michael Catanzaro]

The port-specific differences regarding which libraries are shared and which are static are not really "tamable" in the sense that we don't want to change them. GTK needs JSC to be shared because it's a separate platform library. WPE doesn't expose it, so no need for that. macOS still supports WebKit1 so WebCore has to be sharde there, but we wouldn't that to be shared on Linux. Generally I'd recommend building as much as possible as a static lib, to minimize code size and maximize performance.

If PlayStation doesn't have a really good reason to use shared JSC, I wouldn't. It makes linking a lot harder.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #11)
> The port-specific differences regarding which libraries are shared and which
> are static are not really "tamable" in the sense that we don't want to
> change them. GTK needs JSC to be shared because it's a separate platform
> library. WPE doesn't expose it, so no need for that. macOS still supports
> WebKit1 so WebCore has to be sharde there, but we wouldn't that to be shared
> on Linux. Generally I'd recommend building as much as possible as a static
> lib, to minimize code size and maximize performance.
> 
> If PlayStation doesn't have a really good reason to use shared JSC, I
> wouldn't. It makes linking a lot harder.

We have a good reason for wanting a shared JavaScriptCore 🤐

[Fujii Hironori]

Comment on attachment 389877 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 389877 [details]
Patch

Clearing flags on attachment: 389877

Committed r255905: <https://trac.webkit.org/changeset/255905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.