Summary: | [CMake][Win] Use target oriented design for WebKitLegacy | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||||||
Component: | CMake | Assignee: | 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
Don Olmstead
2019-04-19 12:00:43 PDT
Created attachment 367825 [details]
Patch
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) (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. 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. Created attachment 367847 [details]
Patch
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) (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. Although it looks like this patch doesn't modify any files used by WPE or GTK anyway. 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 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 on attachment 367847 [details] Patch Clearing flags on attachment: 367847 Committed r244488: <https://trac.webkit.org/changeset/244488> All reviewed patches have been landed. Closing bug. |