On Tuesday 04 December 2012, Stephen Kelly wrote:
> Alexander Neundorf wrote:
> > I still think it is not a good idea that
> >
> > target_link_libraries(foo KF5::widgetsaddons)
> >
> > also sets include dirs for the target foo.
> >
> > Should I discuss that again on cmake-devel ?
>
> It would seem more appropriate than here, but I think it's already decided.
Will do so anyway :-)
> >> 4) Also on the topic of naming things correctly, kwidgetsaddons_LIBRARY
> >> should be kwidgetsaddons_LIBRARIES,
> >
> > Yes, I am aware of that, and wanted to change that for all at once.
>
> You mean after committing it as *_LIBRARY everywhere, you want to change it
> all to *_LIBRARIES later? Why not just start with *_LIBRARIES so it doesn't
> have to be change later, or am I missing something?
I just noticed this when I started to write the test-helper (after a bunch of
files), and then decided to change them all at once.
> >> but actually it shouldn't exist at all
> >> because users should use the imported target directly instead as I wrote
> >> above, instead of using the variable.
> >
> > I'm not sold on this.
> > When reading a CMakeLists.txt, I see
> >
> > find_package(Foo)
> >
> > and then I expect to be able to use
> > include_directories(${FOO_INCLUDE_DIRS})
> > ...
> > target_link_libraries(hello ${${FOO_LIBRARIES})
>
> Note that you're not using exact-case in those variables, which I think is
> ill advised anyway.
I can change this to ExactCase, fine with me.
> > Retraining all our users that using include_directories() is wrong
> > doesn't sound good to me.
>
> I think re-training is the cost of simplicity, and it's worth it.
>
> You only expect to have to use include_directories() because you have
> always had to. Soon you won't have to. In the future, people using KF5
> will never have had to.
Yes, well, in the last 5 years many people got used to cmake as it is now.
> >> Additionally, you're setting that
> >> variable to KDE4__kwidgetsaddons, which we'll only have to rename by
> >> hand later.
> >
> > I am aware of that. This was to keep one of those libs compatible with
> > FindKDE4Internal.cmake, I think threadweaver.
> > This is something which can be easily patched with a script.
> >
> >> 5) I also see no need for the kwidgetsaddons_INSTALL_PREFIX variable.
> >
> > In some cases it is necessary, if the user wants/needs to access
> > additional files provided by the package.
>
> If there are additional files provided by the package then we need to
> provide a variable for each of those files, eg
That's also ok.
But this still means it will be just a normal variable pointing to some path,
not some target property.
In this way there is no difference to providing a Foo_INSTALL_PREFIX variable.
> set(Foo_ExtraMacros "${CMAKE_CURRENT_LIST_DIR}/FooExtraMacros.cmake")
>
> and document that variable. We shouldn't expect users to dig into our
> internals like that, nor should we encourage it.
>
> If a file is not documented and has no associated variable, then it is
> fully internal and likely to be changed in non-backward-compatibile ways
> or removed entirely.
>
> Besides, cmake already sets Foo_DIR to the location that the Config file
> was found in.
>
> > It also shouldn't hurt.
>
> It hurts because it is more for us to maintain in 20+ files which is not
> needed or useful.
>
> Let's not add a variable like that to all Config files please. For one
> thing you are adding it to places which certainly don't need it (as they
> have no extra files), and for another it is bad API.
I don't see where this is bad API.
> >> The tier1 Config files (after CMake 2.8.11) should only contain this:
> >> set(kwidgetsaddons_VERSION_MAJOR "@KWIDGETSADDONS_VERSION_MAJOR@")
> >> set(kwidgetsaddons_VERSION_MINOR "@KWIDGETSADDONS_VERSION_MINOR@")
> >> set(kwidgetsaddons_VERSION_PATCH "@KWIDGETSADDONS_VERSION_PATCH@")
> >>
> >> if (NOT TARGET KF5::widgetsaddons) # See item 1.
> >>
> >> include("${CMAKE_CURRENT_LIST_DIR}/kwidgetsaddonsTargets.cmake")
> >>
> >> endif()
> >>
> >> include(FooMacros.cmake) # As required.
> >>
> >> The tier2 and higher config files need also to find_package their
> >> dependencies.
> >>
> >> If you think anything else is needed (apart from for compatibility
> >> variables, which we can provide separately - there is no need for
> >> compatibility variables for kwidgetaddons as it has not existed before),
> >> then please let me know and we can make sure that it can be done with
> >> IMPORTED targets instead.
> >
> > The libraries may have to provide information about supported optional
> > features, but these are typically simple variables, not references to
> > files etc.
>
> Let's deal with situations like that as they come up. Do you know of any
> specific examples?
karchive has optional support for zx and bzip2, users of that library may want
to know that,
Alex
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
