Bug 197112 - [CMake][Win] Use target oriented design for WebKitLegacy
Summary: [CMake][Win] Use target oriented design for WebKitLegacy
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: 196704
  Show dependency treegraph
 
Reported: 2019-04-19 12:00 PDT by Don Olmstead
Modified: 2019-04-20 12:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.88 KB, patch)
2019-04-19 14:08 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (20.73 KB, patch)
2019-04-19 15:41 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.92 MB, application/zip)
2019-04-19 20:22 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-04-19 12:00:43 PDT
Copy the headers. Make includes private. Set a specific derived sources directory.
Comment 1 Don Olmstead 2019-04-19 14:08:07 PDT
Created attachment 367825 [details]
Patch
Comment 2 Konstantin Tokarev 2019-04-19 15:28:56 PDT
Comment on attachment 367825 [details]
Patch

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

> Tools/DumpRenderTree/CMakeLists.txt:25
> +    WTF

This is not correct in ports where WTF is static library which is linked to other target. Instead we should rely on transitive linking feature of CMake, i.e. link only WebKitLegacy here and let it bring in all dependencies whichever are needed (from WebCore, JSC, PAL, WTF)
Comment 3 Don Olmstead 2019-04-19 15:31:32 PDT
(In reply to Konstantin Tokarev from comment #2)
> Comment on attachment 367825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367825&action=review
> 
> > Tools/DumpRenderTree/CMakeLists.txt:25
> > +    WTF
> 
> This is not correct in ports where WTF is static library which is linked to
> other target. Instead we should rely on transitive linking feature of CMake,
> i.e. link only WebKitLegacy here and let it bring in all dependencies
> whichever are needed (from WebCore, JSC, PAL, WTF)

I agree but I want to leave a final cleanup for when everything is done and audit all the libraries.
Comment 4 Konstantin Tokarev 2019-04-19 15:31:58 PDT
While this may seem unneeded given that GTK and WPE bots are green, linking of static libraries WTF into several dynamic objects may cause weird issues later.
Comment 5 Don Olmstead 2019-04-19 15:41:59 PDT
Created attachment 367847 [details]
Patch
Comment 6 Konstantin Tokarev 2019-04-19 17:12:27 PDT
Comment on attachment 367847 [details]
Patch

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

> Source/WebKitLegacy/win/WebKitQuartzCoreAdditions/CMakeLists.txt:45
>  include_directories(

When we finally convert project to target-oriented design, include_directories() and similar command should be gone (and banned by check-webkit-style)
Comment 7 Michael Catanzaro 2019-04-19 17:23:41 PDT
(In reply to Konstantin Tokarev from comment #4)
> While this may seem unneeded given that GTK and WPE bots are green, linking
> of static libraries WTF into several dynamic objects may cause weird issues
> later.

They're probably only green because the EWS only test if it builds, not if it runs. We know from experience that having two copies of WTF is going to break real bad at runtime.
Comment 8 Michael Catanzaro 2019-04-19 17:24:54 PDT
Although it looks like this patch doesn't modify any files used by WPE or GTK anyway.
Comment 9 Build Bot 2019-04-19 20:22:43 PDT
Comment on attachment 367847 [details]
Patch

Attachment 367847 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11937740

New failing tests:
storage/indexeddb/modern/gc-closes-database.html
Comment 10 Build Bot 2019-04-19 20:22:54 PDT
Created attachment 367868 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 WebKit Commit Bot 2019-04-20 12:00:20 PDT
Comment on attachment 367847 [details]
Patch

Clearing flags on attachment: 367847

Committed r244488: <https://trac.webkit.org/changeset/244488>
Comment 12 WebKit Commit Bot 2019-04-20 12:00:22 PDT
All reviewed patches have been landed.  Closing bug.