Using target names directly vs. variables

[Benjamin Port]

On Monday, July 22, 2013 10:34:33 PM David Faure wrote:
> On Sunday 14 July 2013 21:44:38 Alexander Neundorf wrote:
> > On Sunday 14 July 2013, David Faure wrote:
> > > On Sunday 14 July 2013 11:42:26 Alexander Neundorf wrote:
> > > > A good step forward to improve the experience for the user is to
> > > > follow
> > > > those naming conventions as good as possible, so that if you do
> > > > 
> > > > find_package(WhatEverItIs)
> > > > 
> > > > you know, that you can use ${WhateEverItIs_LIBRARIES} and
> > > > ${WhatEverItIs_INCLUDE_DIRS} later on without looking at the
> > > > documentation for this specific module.
> > > 
> > > Yes, this is "the old way" :-)
> > > 
> > > With Qt5 and KF5, however, there's no need for the _INCLUDE_DIRS
> > > variable;
> > > linking to the target name brings the include dirs automatically. It
> > > works
> > > differently, so it's not crazy to use a different syntax for it.
> > > Different feature -> different syntax.
> > > 
> > > To anyone only knowing the "old way", it would actually be quite
> > > confusing
> > > to see cmakelists using ${Foo_LIBRARIES} and NOT using
> > > ${Foo_INCLUDE_DIRS}.
> > > 
> > > We could still set the variables, of course, for the principle of less
> > > surprises you mention, so that app developers can use the variables the
> > > "old way". But this doesn't mean we have to use these variables
> > > internally.
> > 
> > Really, this is not about KDE-only.
> 
> I didn't say KDE anywhere, above.
> 
> > There is, AFAIK, no official "new way".
> > There is, AFAIK, the one documented, standard way, and there is Stephens
> > "let's use the targets directly"-way he just committed without any
> > proposal
> > or discussion about that whatsoever I can remember.
> 
> Well we're discussing it over and over again, it seems :-)
> So let's discuss and finally decide.
> 
CMake allows to use target so it's not about the official way, but more about 
there is this way so we can use it. I think cmake support for targets will not 
be dropped so we can use it.
> > I find it cool that targets can have their include directories attached,
> > no
> > doubt about that.
> > 
> > I have heard so many times the complaint from users, not speaking
> > specifically about KDE developers here, that it sucks that they can't rely
> > on standardized variable names/results from find_package().
> > With using target names directly we make that even worse, instead of
> > improving it.
> > 
> > Then we have three different ways, no standard *at all*:
> > 
> > find_package( Solid )  -> use "KF5::Solid"
> > find_package( JPEG ) -> use ${JPEG_LIBRARIES}
> > find_package( ALSA ) -> use ${ALSA_LIBRARY}
> > 
> > instead of almost standard, with just a few variations:
> > 
> > find_package( Solid )  -> use ${Solid_LIBRARIES}
> > find_package( JPEG ) -> use ${JPEG_LIBRARIES}
> > find_package( ALSA ) -> use ${ALSA_LIBRARY}
> 
> Again, we can provide ${Solid_LIBRARIES} too, for consistency.
I think we already provides this variables for all framewotks. Or we just need 
to fix it
> > Also, there is no guarantee that an imported target actually has its
> > include dirs attached. This also depends on the package in question. So
> > using the target name directly is also no indication that
> > include_directories() is not necessary for this package.
> 
> But for Qt5 and KF5 we can document that they do.
> 
> Killing innovation in the name of consistency is a bit strange --
> consistency is great, so it's being kept (with _INCLUDES and _LIBRARIES
> variables) but at the same time innovation is made available, for the case
> where one finds out in the documentation or in other examples that it's
> available.
> 
Indeed, we can't guarantee that for other libs, but we can add it to framework 
definition of done so we have consistency in all kf5 frameworks.
> > You say "new feature -> new syntax". I would have liked that a lot. David
> > Cole (ex-Kitware) too. Our concerns were dismissed.
> > I would have much prefered if it would have to be explicit that the thing
> > you link against is a target and that you expect it to have its include
> > directories attached. This would have made it obvious to the reader and it
> > would have enabled cmake to generate proper error/warning messages if
> > something goes wrong, something like
> > 
> > target_use_target(hello KF5::Solid  KF5::KArchive)
> 
> Would have worked for me too.
> 
> > Then it would be obvious that those two items are targets, no guessing
> > involved. CMake could also have warned/errored out if those targets would
> > not have include directories attached. It could have errored out if no
> > such
> > targets exist.
> > But it was decided to stay with target_link_libraries(), where any of the
> > arguments can be either a linker flag ("-fPIC"), or a library flag
> > ("-lfoo"), or a target ("Solid") or if no such target exists it's used as
> > the basename of a library (-> "-lSolid").
> 
> This can lead to confusing error messages indeed. But clearly we're
> discussing this in the wrong forum.
> 
> > Using the target names directly also means just that. E.g. until last week
> > in plasma-frameworks the kde4support library was referred to as
> > "KDE4__kde4support". This doesn't look to me like a clean way to use the
> > imported target. The "KDE4__" prefix is actually just an implementation
> > detail, nobody should have to know about. I changed those places to use
> > ${KDE4Support_LIBRARIES}. Now plasma-frameworks is done.
> > Once kde4support can be built separately, no change will be required in
> > plasma-frameworks. It will be possible to build plasma-frameworks both
> > with
> > the all-in-one kdelibs, and also with the separately built frameworks.
> 
> We don't need to support both. At some point the frameworks will be splitted
> out, and then this will be the way to use them...
> 
> > So these variables also add am abstraction layer which protects the users
> > of a package from name changes of the targets in the used project.
> 
> = more magic :)
> 
> > > Well, one needs to find the name of the package too, in the first place.
> > > Should I find_package(attica) or find_package(Attica) or
> > > find_package(LibAttica)?
> > > At some point, someone needs to read some documentation.
> > > In fact, I'm confused. If there's multiple libs in the package (e.g.
> > > kiocore and kiowidgets), with a single find_package(KIO COMPONENTS core
> > > widgets) call, then with variables the same problem happens, surely a
> > > KIO_LIBRARIES isn't good enough.
> > 
> > In KDE we are in a somewhat special situation, especially right now.
> > People working on KDE frameworks now the framework libs and adapt the
> > using
> > modules accordingly.
> > Typically, a user of a library is not a developer of the library, and does
> > not know what the names of the targets are. Typically, he wants to do
> > find_package(Foo) and then target_link_libraries(... ${Foo_LIBRARIES} ).
> 
> This only works if you want to use all libraries from the "package".
> Surely this isn't what you want when the "package" provides multiple libs,
> like Qt does with Qt5Core, Qt5Gui, etc., or Boost and its many libs, or
> ffmpeg, or any other package with "COMPONENTS".
> At least when building more than one executable, you might want to link some
> things to some libs, and other things to other libs.
> 
> > I am not making this up, I have heard that many many times, on mailing
> > lists and in real life.
> > More or less nobody cares whether the build system is cool, for more or
> > less everybody it should just work and get out of the way.
> 
> Stating the obvious? :-)
> This argument is behind everyone's view of it.
> 
> > For KIO, there will be ${KIO_LIBRARIES}, and ${KIO_Core_LIBRARY} etc., as
> > recommended in cmake's readme.txt.
> 
> Ah so it's always <package>_<component>_LIBRARY?
> 
> > > I can write a perl script to replace KService with KF5::KService (and
> > > same
> > > for the other targets) in target_link_libraries lines in 15 minutes
> > > tops.
> > > Changing A to B now so that we don't have to change A to C later doesn't
> > > save anything (same script...).
> > 
> > It doesn't matter whether it takes 15 minutes or more.
> > It is still an effort which is simply not necessary.
> 
> Any change you make now is also an effort. So this is not an argument for
> either way, that was my point.
> 
> > > > * I don't see an advantage in having to teach our developers that for
> > > > KDE
> > > > packages they have to use imported target names (including Qt5), while
> > > > for all other packages they can follow what they know from cmake for
> > > > years, and continue to use ${Foo_LIBRARIES}.
> > > 
> > > See above - it's different because it takes care of the include dirs
> > > too.
> > 
> > If we continue to use the variables, they don't have to learn anything
> > new.
> 
> You're again advocating against innovation in the name of "as we always did
> before". (With such thinking we'd still be using autotools.)
> 
> > If somebody wants to, he can find out that he doesn't need the
> > include_directories() calls anymore. If he doesn't care, everything still
> > works as before. Why change this just for the sake of changing.
> 
> To make it clear why you don't need include_directories(). Otherwise it will
> look like a bug to anyone reading the CMakeLists.txt ("Man, you forgot the
> include_directories statements, this is going to break when the package is
> in a separate install dir!").
> 
> Different feature -> different syntax -> no ambiguity.
> 
> > > > * I have never heard a complaint from a cmake user that he can't use
> > > > imported target names directly but has to use variables for external
> > > > packages. So I don't see any motivation to change this.
> > > 
> > > The complaint is: if I use a variable that is not set, it is expanded to
> > > nothing, and all I get is undefined symbols. If I use a target name that
> > > is
> > > unknown, I get an error about that very target name, much easier to
> > > debug.
> > 
> > I basically asked for a complaint, so you found something, no surprise.
> > Nobody ever was annoyed by this enough (or at all) to complain about that.
> > Really.
> 
> They didn't know a better solution would be available one day.
> 
> > If you get undefined references to solid_something, and the
> > target_link_libraries() call contains ${Solid_LIBRARIES}, it seems quite
> > obvious to me where to start looking.
> 
> You assume very clear symbol names. They aren't always so clear.
> Undefined reference to ucal_open_49 -- which lib should you link to?
> 
> > I would have a different opinion if there was a direct match between
> > package name and target names. There isn't, not even in KF5 ("Solid" ->
> > "KF5::Solid").
> 
> That's really a direct match, isn't?
> Maybe we can name the package name KF5Solid? Like Qt names it Qt5Core?
> (right?)
> 
> In the end, you and Stephen can decide, I'm "just a user" in the respect of
> cmake files, you guys are the maintainers. I'm just trying to help moving
> the discussion along. But a consensus would be good soon, because Benjamin
> is already asking which form to use and standardize upon in his cleanups.
We ned to had it to the definition of done, and rework on already done 
framework in order to have consistency between all frameworks.

--
Benjamin Port