Config files for frameworks

[Stephen Kelly]

Alexander Neundorf wrote:
>> > 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.

People still get include_directories() calls wrong all the time. By having 
that information on the target, that will be a problem of the past. I also 
think it's redundant information to write 

 include_directories(${foo_INCLUDE_DIRS})

everywhere - if you're linking to libfoo you need to compile with its 
headers and defines available first.

There is no point in keeping cmake files looking exactly as they do now if 
we can instead remove noisy/redundant information. That is progress. If it 
was 2006 what would you choose?

Config files are not very commonplace. People are 'used to' Find files. 
Using Config files is progress and so we do that. This is the same kind of 
progress. People will get used to Config files and they will get used to not 
needing to specify and maintain include_directories information and 
COMPILE_DEFINITIONS.

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

Yes.

> In this way there is no difference to providing a Foo_INSTALL_PREFIX
> variable.

I disagree. 

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

In most cases it is not useful because Foo_DIR is already set, and because 
no 'extra files' are available anyway (Do you have a case where such extra 
files are available?). That's already a reason it is bad API (defined but 
not useful). 

The second reason is that any files which happen to be in the directory, but 
are undocumented, are internal, and not for downstreams to include(). Always 
using variables as QT_USE_FILE does for example (and documenting the 
variables) is better.

It's also shorter:

 # Do I need to wrap this in "" ?
 include(${Foo_INSTALL_PREFIX}/FooExtraSettings.cmake) 

 include(${Foo_EXTRA_SETTINGS})

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

Good example. Let's follow up with that later. 

I'm interested in concluding the rest of this mail though.

Thanks,

Steve.