[CMake] WebKit should link to WebCore as a PRIVATE library if WebCore is a static library

• Component: WebKit2
• Status: RESOLVED FIXED

[Fujii Hironori]

[CMake] WebKit should link to WebCore as a PRIVATE library

While trying to build TestWebKit for Windows port (Bug 174003), I see the following linkage error.

> ------ Build started: Project: TestWebKitLib, Configuration: Debug x64 ------
> WebCore.lib(UnifiedSource311.obj) : error LNK2005: "struct WTFLogChannel * __cdecl WebCore::notImplementedLoggingChannel(void)" (?notImplementedLoggingChannel@WebCore@@YAPEAUWTFLogChannel@@XZ) already defined in WebKit2.lib(WebKit2.dll)
>    Creating library C:/webkit/gb/WebKitBuild/Debug/lib64/TestWebKitLib.lib and object C:/webkit/gb/WebKitBuild/Debug/lib64/TestWebKitLib.exp
> C:\webkit\gb\WebKitBuild\Debug\bin64\TestWebKitLib.dll : fatal error LNK1169: one or more multiply defined symbols found
> Done building project "TestWebKitLib.vcxproj" -- FAILED.
> ========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

This happens because TestWebKitLib links to both WebCore and WebKit.
But, TestWebKitLib explicitly links only with WebKit,
WebCore is propagated because WebKit links WebCore as PUBLIC.

[Fujii Hironori]

Created attachment 336754 [details]
WIP patch

This patch doen't work for ports using generate-forwarding-headers.pl.
Because generate-forwarding-headers.pl creates forwarding headers only for headers included by WebKit2.
Then, include paths should contain all WebCore directories.

[Fujii Hironori]

Created attachment 336762 [details]
WIP patch

[Fujii Hironori]

Created attachment 336836 [details]
WIP patch

[Fujii Hironori]

Created attachment 336840 [details]
Patch

[Michael Catanzaro]

Let's see what Konstantin thinks.

[Konstantin Tokarev]

This change is wrong for cases when WebCore is built as SHARED library, because all users of WebKit need to link WebCore as well

[Fujii Hironori]

Created attachment 336969 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 336969 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1994
> +    target_link_libraries(WebCore PUBLIC ANGLESupport)

ANGLESupport must be PRIVATE

> Source/WebKit/PlatformGTK.cmake:461
> +list(APPEND WebKit_PRIVATE_LIBRARIES

There is no real need for new variable - just add 'PUBLIC' and 'PRIVATE' keywords to WebKit_LIBRARIES.

[Fujii Hironori]

Comment on attachment 336969 [details]
Patch

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

>> Source/WebCore/CMakeLists.txt:1994
>> +    target_link_libraries(WebCore PUBLIC ANGLESupport)
> 
> ANGLESupport must be PRIVATE

No.
I tried PRIVATE, but I got compilation errors. I'll attach the error message for the records, tomorrow.

>> Source/WebKit/PlatformGTK.cmake:461
>> +list(APPEND WebKit_PRIVATE_LIBRARIES
> 
> There is no real need for new variable - just add 'PUBLIC' and 'PRIVATE' keywords to WebKit_LIBRARIES.

I think I need to switch back to PUBLIC again.

> list(APPEND WebKit_PRIVATE_LIBRARIES PRIVATE WebCore PUBLIC)

I'll remake the patch.

[Konstantin Tokarev]

Comment on attachment 336969 [details]
Patch

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

>>> Source/WebCore/CMakeLists.txt:1994
>>> +    target_link_libraries(WebCore PUBLIC ANGLESupport)
>> 
>> ANGLESupport must be PRIVATE
> 
> No.
> I tried PRIVATE, but I got compilation errors. I'll attach the error message for the records, tomorrow.

Ah, that's because of include directories. Same problem as what you are doing with WebCoreHeaderInterface

[Fujii Hironori]

Comment on attachment 336969 [details]
Patch

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

>>> Source/WebKit/PlatformGTK.cmake:461
>>> +list(APPEND WebKit_PRIVATE_LIBRARIES
>> 
>> There is no real need for new variable - just add 'PUBLIC' and 'PRIVATE' keywords to WebKit_LIBRARIES.
> 
> I think I need to switch back to PUBLIC again.

GTK port is using ADD_WHOLE_ARCHIVE_TO_LIBRARIES.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/PlatformGTK.cmake?rev=228590#L668
Do you think I should modify ADD_WHOLE_ARCHIVE_TO_LIBRARIES to remove PUBLIC and PRIVATE keywords?

[Fujii Hironori]

Comment on attachment 336969 [details]
Patch

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

>>>> Source/WebCore/CMakeLists.txt:1994
>>>> +    target_link_libraries(WebCore PUBLIC ANGLESupport)
>>> 
>>> ANGLESupport must be PRIVATE
>> 
>> No.
>> I tried PRIVATE, but I got compilation errors. I'll attach the error message for the records, tomorrow.
> 
> Ah, that's because of include directories. Same problem as what you are doing with WebCoreHeaderInterface

I tried again and no problem happens today. I'll make ANGLESupport PRIVATE.

[Fujii Hironori]

Created attachment 337050 [details]
Patch

[Fujii Hironori]

Created attachment 337138 [details]
Patch

[Fujii Hironori]

Hi Konstantin, I addressed the review feedbacks. Could you review again?

[Konstantin Tokarev]

Comment on attachment 337138 [details]
Patch

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

> Source/WebKit/CMakeLists.txt:726
> +    set(WebKit_LIBRARIES WebCoreHeaderInterface PRIVATE WebCore PUBLIC)

While this line is syntactically correct, I think we should write such code like

   set(WebKit_LIBRARIES
     PUBLIC WebCoreHeaderInterface
     PRIVATE WebCore
   )

i.e. all libraries/targets have their "access specifiers" in front of them, instead of providing access specifier for unknown follow-up list

[Fujii Hironori]

Created attachment 337327 [details]
Patch

[Fujii Hironori]

Comment on attachment 337327 [details]
Patch

Thank you for the review, Konstantin. I did it.

[Konstantin Tokarev]

Comment on attachment 337327 [details]
Patch

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

> Source/WebKit/PlatformGTK.cmake:463
> +    PRIVATE ${GTK_UNIX_PRINT_LIBRARIES}

Don't repeat PRIVATE

 list(APPEND WebKit_LIBRARIES
    PRIVATE
      A
      B
      C
    PUBLIC
      ...
  )

[Fujii Hironori]

Created attachment 337354 [details]
Patch

[Fujii Hironori]

Committed r230385: <https://trac.webkit.org/changeset/230385>

[Radar WebKit Bug Importer]

<rdar://problem/39270000>