Bug 197112

Summary: [CMake][Win] Use target oriented design for WebKitLegacy
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: CMakeAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, commit-queue, ews-watchlist, Hironori.Fujii, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 196704    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews206 for win-future none

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 EWS Watchlist 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 EWS Watchlist 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.