Bug 207004

Created attachment 389266 [details] Patch

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

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?

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

>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

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

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

(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

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

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.

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

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

Created attachment 389315 [details] Patch I changed over all cases where PUBLIC or PRIVATE was specified when using _LIBRARIES.

Created attachment 389317 [details] Patch

Comment on attachment 389317 [details] Patch Clearing flags on attachment: 389317 Committed r255491: <https://trac.webkit.org/changeset/255491>

All reviewed patches have been landed. Closing bug.