Config files for frameworks (Was: KArchive for Qt4)

[Alexander Neundorf]

On Thursday 29 November 2012, Stephen Kelly wrote:
> Alexander Neundorf wrote:
> > The code for creating a Config.cmake file is not trivial, but IMO also
> > not boilerplate, and Stephen agreed in Berlin that this will have to be
> > done individually be every project. This is the added
> > threadweaverConfig.cmake.in and the call to
> > configure_package_config_file() in the CMakeLists.txt.
> 
> I think it's premature to be creating and checking in Config files for
> frameworks.

I don't think so.
It gets the buildsystem work at least going.
It gives me/us a chance to actually test the exporting into multiple exports 
etc.
It gives us the chance to clean up the buildsystem a bit from its current 
state and get it closer to what it will be like, and gather input.

> As they are not generated, doing that now will mean that ~20 files will
> need to be changed if something is wrong.

Yes.
That's the price we have to pay for modularization I'm afraid.
 
> There are already several problems with the files you're checking in now.
> 
> 1) The inclusion of the Targets.cmake file is unguarded:
> 
> See http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/5410

Thanks.

> 2) You are setting things that do not need to be set:
> 
>  * Eg: kwidgetsaddons_INCLUDE_DIR, kwidgetsaddons_LIBRARY_DIR.
> 
> The kwidgetsaddons_LIBRARY_DIR is not needed at all. That's the kind of
> thing that you only need (and put in the cache) if you're writing a Find
> file so that the user can 'help' and update the cache to the actual
> directory if needed
> 
>  http://osdir.com/ml/programming.tools.cmake.user/2006-10/msg00152.html
> 
> As we're providing Config files, the user is not going to know better than
> the config file "by design", so we don't need to set the _DIR variable.

Yes, I agree.

...
> Additionally, when we depend on CMake 2.8.11, we won't need to set the
> kwidgetsaddons_INCLUDE_DIRS variable, because the contents of that will be
> encoded into the IMPORTED target, and the user will simply do
> 
>  target_link_libraries(foo KF5::widgetsaddons)
> 
> or simlilar without needing to specify the include dirs manually. If you
> add those things now, we'll just have to remove them when we update the
> CMake dependency.

I know about the history of this syntax, so...

I still think it is not a good idea that 

target_link_libraries(foo KF5::widgetsaddons)

also sets include dirs for the target foo.
No average user will figure out where the include dirs used for the target foo 
come from, because this looks just like the well known target_link_libraries() 
call known for a long time.

Should I discuss that again on cmake-devel ?
I would very much prefer a 
target_(link|uses)_targets(foo KF5::widgetsaddons)
which would make every average cmake users go and have a look at the man page, 
what this does. It would also fail on non-targets.

And users could decide whether they want to use

include_directories(${Foo_INCLUDE_DIRS})
add_executable(hello ${srcs})
target_link_libraries(hello ${Foo_LIBRARIES})

or the new style

add_executable(hello ${srcs})
target_uses_targets(hello ${Foo_LIBRARIES})


> 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.

> 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})

Retraining all our users that using include_directories() is wrong doesn't 
sound good to me.


> 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.
It also shouldn't hurt.
 
> 6) You've already made some copypastos:
> 
> +set(kwidgetsaddons_VERSION_MAJOR "@KWIDGETSADDONS_VERSION_MAJOR@")
> +set(kwidgetsaddons_VERSION_MINOR "@KWIDGETSADDONS_VERSION_MINOR@")
> +set(kwindowsystem_VERSION_PATCH "@KWIDGETSADDONS_VERSION_PATCH@")

Thanks.
I already started to write something to make testing these files easier.

> Given that we'll need to maintain Config files by hand, let's please:
> 
> a) minimise what we put into them (no need to set variables which are not
> useful, and which we run the risk of typo-ing),
> b) Make sure that what we write by hand is as close to the final state as
> possible. We don't want to have to manually update all KDE4__* to KF5::*
> etc.

Now this one is really a quick scriptable change, no problem there.
 
> 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.

Alex