Bug 197112 – [CMake][Win] Use target oriented design for WebKitLegacy

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

Copy the headers. Make includes private. Set a specific derived sources directory.

[Don Olmstead]

Created attachment 367825 [details]
Patch

[Konstantin Tokarev]

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)

[Don Olmstead]

(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.

[Konstantin Tokarev]

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.

[Don Olmstead]

Created attachment 367847 [details]
Patch

[Konstantin Tokarev]

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)

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Although it looks like this patch doesn't modify any files used by WPE or GTK anyway.

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 367847 [details]
Patch

Clearing flags on attachment: 367847

Committed r244488: <https://trac.webkit.org/changeset/244488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.