> 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. > > Michael Palimaka wrote: > 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. > > Alex Merry wrote: > Yes, that's exactly it. Sorry for not being clearer. > > Michael Palimaka wrote: > So, should the explicit link be added, or just forget about it for now? > :-)
I would prefer it if you either added explicit links or removed the find_package calls I highlighted, but if you would rather not, I'm still willing to give a ship it. - Alex ----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
