Config files for frameworks (Was: KArchive for Qt4)

[Alexander Neundorf]

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