Bug 207004 – [CMake] Add _PRIVATE_LIBRARIES to framework

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Don Olmstead]

Currently there's no differentiation between PRIVATE libraries and PUBLIC. If a port wants to list a library as PRIVATE they have to add it for each library which isn't ideal.

[Don Olmstead]

Created attachment 389266 [details]
Patch

[Konstantin Tokarev]

If this is about port-specific libraries, I would prefer you'd rather used target_link_libraries in port-specific file instead of adding new "magic" variable

[Michael Catanzaro]

So WebKit's current CMake coding style is to use these magic variables rather extensively. I'm not a huge fan of it personally, but it works and this seems entirely consistent with existing practice. And consistency is good. Any reasoning for why you'd prefer target_link_libraries() in port-specific files? Currently we generally avoid doing that manually in preference to these magic variables, right?

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #2)
> If this is about port-specific libraries, I would prefer you'd rather used
> target_link_libraries in port-specific file instead of adding new "magic"
> variable

Its not about port specific libraries. See https://github.com/WebKit/webkit/blob/master/Source/WebKit/CMakeLists.txt#L219-L232

```
if (${WebCore_LIBRARY_TYPE} STREQUAL STATIC)
    # Link with WebCore as PRIVATE not to propagate WebCore to all users of WebKit.
    set(WebKit_LIBRARIES
        PRIVATE WebCore
        PUBLIC WebCoreHeaderInterface
    )
else ()
    # All users of WebKit need to link WebCore if WebCore is a shared library.
    if (APPLE)
        set(WebKit_LIBRARIES PRIVATE WebCore)
    else ()
        set(WebKit_LIBRARIES PUBLIC WebCore)
    endif ()
endif ()
```

This sort of stuff needs to be done properly throughout.

[Konstantin Tokarev]

>Any reasoning for why you'd prefer target_link_libraries() in port-specific files?

To encourage more furture uses of target-specific commands in port-specific files instead of adding more and more variables

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #5)
> >Any reasoning for why you'd prefer target_link_libraries() in port-specific files?
> 
> To encourage more furture uses of target-specific commands in port-specific
> files instead of adding more and more variables

I don't realistically see WEBKIT_FRAMEWORK macros going away in the near term. There's plenty of goofy shit going on especially with how dlls are used for all the executables on Windows.

The problem with _LIBRARIES is if a port decides to specify PUBLIC or PRIVATE it has a consequence. So with the above example if someone just appends to WebKit_LIBRARIES without a PUBLIC/PRIVATE then it gets whatever the last modules privacy is. See every other Platform*.cmake in Source/WebKit which just appends there like every other thing.

This is step 1 to clean that up. If in the end we want to do a find/replace of `list(APPEND WebKit_LIBRARIES` to do `target_link_libraries(WebKit PUBLIC ` thats fine but at the moment everything is busted.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #6)
> This is step 1 to clean that up. If in the end we want to do a find/replace
> of `list(APPEND WebKit_LIBRARIES` to do `target_link_libraries(WebKit PUBLIC
> ` thats fine but at the moment everything is busted.

Honestly it seems like a pragmatic step forward to me.

[Konstantin Tokarev]

(In reply to Don Olmstead from comment #6)
> The problem with _LIBRARIES is if a port decides to specify PUBLIC or
> PRIVATE it has a consequence. So with the above example if someone just
> appends to WebKit_LIBRARIES without a PUBLIC/PRIVATE then it gets whatever
> the last modules privacy is. See every other Platform*.cmake in
> Source/WebKit which just appends there like every other thing.

If we avoid using WebKit_LIBRARIES and use target_link_libraries() everywhere, things are enforced to be correct, as each target_link_libraries() call doesn't effect others. OTOH, using variables like WebKit_PRIVATE_LIBRARIES is not failsafe, as it's possible to insert PUBLIC there by mistake and rest of list will be borked

[Konstantin Tokarev]

Also, my main issue with this patch is that it just introduces new variable without showing any place where it's going to be used. This makes it hard to see if it's a sensible intermediate state or just wandering around, instead of e.g. going straight to replacing all uses of list(APPEND x_LIBRARIES ...) to target_link_libraries(x ...)

[Konstantin Tokarev]

Comment on attachment 389266 [details]
Patch

On the second thought, I still think it shouldn't go in as is. if/else branch depending on state of ${_target_logical_name}_PRIVATE_LIBRARIES variable may lead to hidden bugs for targets (or cases, as it tests emptiness instead of definedness) when variable is empty, while in opposite cases it doesn't check that ${_target_logical_name}_LIBRARIES} does not contain PUBLIC/PRIVATE keywords. So we have more variants of behavior with more kinds of possible errors.

[Konstantin Tokarev]

To be safe, it should test presence of keywords in ${_target_logical_name}_LIBRARIES}, if nothing found use new code path, otherwise old one.

[Konstantin Tokarev]

It could also print a warning if "legacy" path is still taken anywhere, before it's finally removed

[Don Olmstead]

Created attachment 389315 [details]
Patch

I changed over all cases where PUBLIC or PRIVATE was specified when using _LIBRARIES.

[Don Olmstead]

Created attachment 389317 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389317 [details]
Patch

Clearing flags on attachment: 389317

Committed r255491: <https://trac.webkit.org/changeset/255491>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.