Bug 198493 - [CMake] Add WebKit::PAL target
Summary: [CMake] Add WebKit::PAL target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-03 13:17 PDT by Don Olmstead
Modified: 2020-02-27 14:37 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.61 KB, patch)
2019-06-03 13:24 PDT, Don Olmstead
annulen: review+
Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2019-06-03 15:40 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (6.51 KB, patch)
2019-06-04 12:43 PDT, Don Olmstead
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.55 MB, application/zip)
2019-06-04 14:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.01 MB, application/zip)
2019-06-04 19:48 PDT, EWS Watchlist
no flags Details
WIP Patch (5.96 KB, patch)
2020-02-26 17:33 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (5.96 KB, patch)
2020-02-26 17:49 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (5.96 KB, patch)
2020-02-26 18:15 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (8.91 KB, patch)
2020-02-26 18:24 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2020-02-27 12:27 PST, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-06-03 13:17:59 PDT
Add WebKit::PAL target
Comment 1 Don Olmstead 2019-06-03 13:24:30 PDT Comment hidden (obsolete)
Comment 2 Don Olmstead 2019-06-03 15:40:11 PDT Comment hidden (obsolete)
Comment 3 Don Olmstead 2019-06-04 12:43:56 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-06-04 14:40:32 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-06-04 14:40:34 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-06-04 19:48:00 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-06-04 19:48:02 PDT Comment hidden (obsolete)
Comment 8 Don Olmstead 2020-02-26 17:33:06 PST Comment hidden (obsolete)
Comment 9 Don Olmstead 2020-02-26 17:49:45 PST Comment hidden (obsolete)
Comment 10 Don Olmstead 2020-02-26 18:15:09 PST Comment hidden (obsolete)
Comment 11 Don Olmstead 2020-02-26 18:24:03 PST Comment hidden (obsolete)
Comment 12 Don Olmstead 2020-02-27 12:27:48 PST
Created attachment 391897 [details]
Patch
Comment 13 Michael Catanzaro 2020-02-27 13:11:14 PST
Comment on attachment 391897 [details]
Patch

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

targets :D

> Source/cmake/target/PAL.cmake:14
> +    # Just assuming Windows for the moment
> +    add_library(WebKit::PAL STATIC IMPORTED)
> +    set_target_properties(WebKit::PAL PROPERTIES
> +        IMPORTED_LOCATION ${WEBKIT_LIBRARIES_RUNTIME_DIR}/PAL${DEBUG_SUFFIX}.dll
> +        IMPORTED_IMPLIB ${WEBKIT_LIBRARIES_LINK_DIR}/PAL${DEBUG_SUFFIX}.lib
> +        # Should add Apple libraries here when https://bugs.webkit.org/show_bug.cgi?id=205085 lands
> +        INTERFACE_LINK_LIBRARIES "WebKit::WTF"
> +    )

Unguarded Windows stuff in a cross-platform file doesn't seem good.
Comment 14 Michael Catanzaro 2020-02-27 13:12:48 PST
Comment on attachment 391897 [details]
Patch

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

> Source/cmake/target/PAL.cmake:4
> +if (NOT TARGET WebKit::PAL)
> +    if (NOT INTERNAL_BUILD)
> +        message(FATAL_ERROR "WebKit::PAL target not found")
> +    endif ()

I see, I missed that we'll never reach the Windows-specific stuff.
Comment 15 Fujii Hironori 2020-02-27 13:13:27 PST
Comment on attachment 391897 [details]
Patch

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

LGTM2

> Source/WebCore/PAL/pal/CMakeLists.txt:43
> +# linked otherwise it can be linked directly

Theoretically, this also should be wrapped into the interface library WebKit::WTF, shound't it?
Comment 16 Don Olmstead 2020-02-27 13:53:19 PST
(In reply to Fujii Hironori from comment #15)
> Comment on attachment 391897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391897&action=review
> 
> LGTM2
> 
> > Source/WebCore/PAL/pal/CMakeLists.txt:43
> > +# linked otherwise it can be linked directly
> 
> Theoretically, this also should be wrapped into the interface library
> WebKit::WTF, shound't it?

WebKit::WTF is used for building JSC and some test stuff. I don't think you can technically modify it to include JSC without that part of the build freaking out.

After writing that if statement to make the decision of what to link to PAL I started thinking of how a CMake macro or function is really needed for anything trying to link a WebKit framework. The complexity is because we have so many different variants of the framework libraries.

Mac/PlayStation - STATIC bmalloc/WTF exposed through SHARED JSC
Windows - SHARED WTF and JavaScriptCore
WPE - STATIC everything except WebKit

I think the best way forward is to have a function/macro for when you link a WebKit framework. So the above would be WEBKIT_FRAMEWORK_LINK(PAL WTF) and under the covers it would figure that out and provide book keeping on what it is you're supposed to link in the end.
Comment 17 WebKit Commit Bot 2020-02-27 14:37:07 PST
Comment on attachment 391897 [details]
Patch

Clearing flags on attachment: 391897

Committed r257587: <https://trac.webkit.org/changeset/257587>
Comment 18 WebKit Commit Bot 2020-02-27 14:37:09 PST
All reviewed patches have been landed.  Closing bug.