Review Request 115863: Improve khtml dependencies

[Michael Palimaka]

> On Feb. 19, 2014, 3:21 p.m., Alex Merry wrote:
> > CMakeLists.txt, lines 23-32
> > <https://git.reviewboard.kde.org/r/115863/diff/1/?file=244733#file244733line23>
> >
> >     KCompletion, KConfig[Widgets] and KCoreAddons are used, but never linked against.  So there's not much point searching for them: we're already depending on them being bought in by other libraries.
> 
> Michael Palimaka wrote:
>     The listed frameworks are directly used. I don't see how linking or not makes a difference - khtml will fail to build without them. If we trust that one dependency will always bring in some other dependencies that we happen to also use, I am sure there are others we could remove too.
> 
> Alex Merry wrote:
>     Well, my view is that you can either find them and link against them (explicit dependencies), or not find or link against them (implicitly trust that the libraries you *do* link against bring them in).  Finding them and not linking against them just seems a bit pointless.
> 
> Michael Palimaka wrote:
>     A dependency can still be explicit even though a binary link isn't required, for example:
>     
>     src/rendering/render_form.cpp:#include <kservice.h>
>     src/rendering/render_form.cpp:            KService::List providers = KServiceTypeTrader::self()->query("SearchProvider");
>     src/rendering/render_form.cpp:            foreach (const KService::Ptr &provider, providers) {
>     
>     If you feel that strongly about it though, I'll drop those changes. They additions don't affect me at all, I just was aiming for a 'correct' change since I was touching this framework anyway.
> 
> Alex Merry wrote:
>     Yes, but this code won't work unless you link against KF5::Service, either explicitly or implicitly.  If it's explicit, you should also have find_package(KF5Service).  If it's implicit, you're depending on one of your explicit dependencies including KF5::Service in its link interface anyway, in which case that dependency will also pull in the KF5Service package.
>     
>     I mean, in the end it doesn't matter that much.  I'm not going to say "don't ship it unless you do this".  But I think that searching for packages in CMake and then not making any use of those packages in CMake (even if it doesn't make any difference to whether the package is required) just clutters things up unnecessarily.

Sorry, I just realised I misunderstood your original comment. The final product does actually link to KCompletion etc. but indeed they are not explicitly marked that way. I guess for the case of KCompletion the link is happening anyway because it is a public dependency of KTextWidgets.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115863/#review50260
-----------------------------------------------------------


On Feb. 18, 2014, 8:01 a.m., Michael Palimaka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115863/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2014, 8:01 a.m.)
> 
> 
> Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: khtml
> 
> 
> Description
> -------
> 
> - QtTest is only required for autotests
> - QtX11Extras is only required for X11 builds, and for tests
> - Remove KCrash, KDBusAddons, KGuiAddons, KInit, and KItemViews as they are not used
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 3a3dbab90e6572cf953ba5edc1fcb60a7e30b493 
>   autotests/CMakeLists.txt 33f1ecb7ba65f223baef242eb21cd34457b020da 
>   tests/CMakeLists.txt 8fc438fa932ec43492b6c2a8e5bc8f0337550d1a 
> 
> Diff: https://git.reviewboard.kde.org/r/115863/diff/
> 
> 
> Testing
> -------
> 
> Builds. Tests pass.
> 
> 
> Thanks,
> 
> Michael Palimaka
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140220/a30b948f/attachment-0001.html>