Config files for frameworks

[Alexander Neundorf]

On Tuesday 04 December 2012, Stephen Kelly wrote:
> 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?

I would prefer if both ways would stay valid and continue to work as they do 
now:

include_directories(${Foo_INCLUDE_DIRS})
add_executable(hello main.cpp)
target_link_libraries(hello ${Foo_LIBRARIES})

as well as 

add_executable(hello main.cpp)
target_use_targets(hello Foo::FooLibrary)


This would make obvious what's going on, and not change the behaviour of an 
existing command.
The second one might even warn if the used target doesn't have include 
directories set etc.
(I haven't actually checked your implementation, sorry if this is already the 
case)

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

It's not like this would be a fixed, written down rule.
Even if it was, not everybody might know or follow that rule.
I don't think providing this PREFIX variable or not is a significant issue.

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

I don't disagree.

> >> >> 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.
> 
> 
> _______________________________________________
> Kde-frameworks-devel mailing list
> Kde-frameworks-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kde-frameworks-devel