Fix iCub config

[Nicogene]

Since #652 find_package(ICUB) shadows previous find_package(YARP) calls, this PR fix this problem.

Please review code.

[pattacini]

LGTM 🚀

[traversaro]

Ack for this specific change, but to be honest this seems to be a serious usability issue at the YARP CMake level, what do you think @drdanz ?

[Nicogene]

Fyi this workaround was already present https://github.com/robotology/icub-main/blob/master/conf/template/icub-config-install.cmake.in#L9

[drdanz]

I'm sorry if I'm late, but the patch is wrong.

TL;DR

YARP_LIBRARIES should not be used. Appending targets to the YARP_LIBRARIES is just evil. Either restore it to its previous value, or don't touch it. The first option makes, more sense, but it makes iCub behave differently compared to any other library using YARP.

Explaination:

A user using ONLY iCub libraries, should NOT link explicitly to any YARP target. If YARP libraries are required for compiling/linking a target using iCub libraries, these should be in the INTERFACE_LINK_LIBRARIES property for iCub target, but the user should not know anything about that.

If the user is using BOTH YARP and iCub libraries, he should be linking the YARP libraries he is using, but he should not link explicitly the ones required to use iCub targets.

As I've been stressing out for several years now, YARP_LIBRARIES should not be used, and will be deprecated at some point in favour of targets, which are the recommended way to use CMake libraries at least since CMake 3.0 (and probably before) i.e. since 2014.

Also, as documented in YARP 3.0.0 release notes (i.e. when components were introduced), the YARP_LIBRARIES variable always contains the components searched in the last find_package(YARP) call, therefore if you call find_dependency(YARP), the YARP_LIBRARIES variable will contain whatever components you searched for. This was a breaking change and as a consequence it was introduced with a major release, and it has been this way since then.

You can now make several find_package(YARP) with different components in different places of your code/directories, and you will always be using the libraries that you requested, even if you are using the YARP_LIBRARIES variable.

Changing the value of this variable inside ICUBConfig.cmake is very wrong. Either restore it with the value that it had before calling find_package(ICUB), or don't touch it at all. If you change it, a user that expects to have a few components there, will link with all the YARP libraries searched by ICUB, even if he is not using them, and these are not in any INTERFACE_LINK_LIBRARIES of the targets used.

The user knows which of the YARP libraries are used in his code, not ICUBConfig.cmake.

Also if you change the order of the find_package(ICUB) followed by find_package(YARP), the variable will contain only the libraries searched by the latest call.

Discussion

This is not the actual usability issue, because at this point, any YARP user should be aware that he should be using targets instead of the YARP_LIBRARIES variable, and this will just help him understand that he is doing something wrong.

I think that the issue comes from this change in CMake 3.15, and not from #652. The actual change is something like this:

--- a/Modules/CMakeFindDependencyMacro.cmake
+++ b/Modules/CMakeFindDependencyMacro.cmake
@@ -31,7 +31,6 @@ CMakeFindDependencyMacro
 #]=======================================================================]
 
 macro(find_dependency dep)
-  if (NOT ${dep}_FOUND)
   set(cmake_fd_quiet_arg)
   if(${CMAKE_FIND_PACKAGE_NAME}_FIND_QUIETLY)
     set(cmake_fd_quiet_arg QUIET)
@@ -61,5 +60,4 @@ macro(find_dependency dep)
   endif()
   set(cmake_fd_required_arg)
   set(cmake_fd_quiet_arg)
-  endif()
 endmacro()

The actual problem is that find_dependency is a macro and not a function, and therefore it does not add a variable scope, which means any package searching for YARP will overwrite this variable, making the meaning of this variable very random, depending on what was called between the moment when find_package(YARP), and when the library is actually used.

The same issue is valid for any other variable defined inside the config file, and that's yet another reason to prefer targets to variables.

I would recommend once again to deprecate this variable once and for all.

[pattacini]

Thanks @drdanz for the very detailed explanation and useful insights 👍

Although it's clear that this approach is far from being optimal and very likely only temporary, this PR aimed to just reinstate an old patch that was there for long as @Nicogene pointed at above.

It's fundamental for us to make sure that our huge legacy code won't be broken. This particular condition made actually the compilation of a couple of cer modules fail.

We will suitably deal with this at the right time and at a larger scale.

[Nicogene]

Thanks @drdanz for your digression , what you are saying is correct, using the explicit target respect the *_LIBRARIES variable is better, because it makes clear what your module or library need, it is the same of including the headers you need respect to the all.h.
On the other hand it reduces the accessibility of using the repository, using the targets is more difficult thant blindly using the *_LIBRARIES var. So I think that is a trade-off of what is more correct and what is more user-friendly

The great majority of our community uses cmake in a very basic way, they are not very aware about how the targets works, and which are the differences/advantages of using them explicitly.
I undestand an high-level user that doesn't want to spend too much time struggling with cmake, in this sense using the *_LIBRARIES variable is very useful.
But I understand an expert user like you that think that using that variable is messy.

A user using ONLY iCub libraries, should NOT link explicitly to any YARP target. If YARP libraries are required for compiling/linking a target using iCub libraries, these should be in the INTERFACE_LINK_LIBRARIES property for iCub target, but the user should not know anything about that.

This is true, but the status of libraries in icub-main is very eterogenous, in some is correctly explicited, in some other they just link ${YARP_LIBRARIES}.
I think that the first step is to do an internal reorder and cleanup, that can be addressed in the relative near future.

[traversaro]

To be honest I tough the PR was restoring the old value of YARP_LIBRARIES, not appending it, sorry for not checking thoroughly.

For the usability problem, the point I was referring to is simply that after https://gitlab.kitware.com/cmake/cmake/-/merge_requests/3161 if I call:

find_package(YARP COMPONENTS theComponentsThatIUse)
find_package(ICUB)

I would expect that the YARP_LIBRARIES variable contains the requested components, while in this case the fact that ICUB is calling find_dependency(YARP) internally, is perturbing the value of YARP_LIBRARIES. This means that any downstream library that is using YARP can't just call find_dependency(YARP), but rather it needs to store and then restore the value of YARP_LIBRARIES, complexifing considerably the process of writing the LibraryThatDependsOnYARPConfig.cmake file. One possible way to proceed is to make sure that no one is using YARP_LIBRARIES and so the downstream users can be ignore the problem.

The same issue is valid for any other variable defined inside the config file, and that's yet another reason to prefer targets to variables.

Not really, to be honest. It is only a problem for variables whose content depend on the arguments passed to the find_dependency call, and in the case of dependencies it is typically just COMPONENTS. For example, projects that just define <package>_LIBRARIES as all the targets exported by the package are not affected by that.

[Nicogene]

I have just restored the old behavior: https://github.com/robotology/icub-main/blob/master/conf/template/icub-config-install.cmake.in#L47

Also before it was appending YARP_LIBRARIES, shall we change it in order to restore the original content before the find_package(ICUB) ?

[pattacini]

Also before it was appending YARP_LIBRARIES, shall we change it in order to restore the original content before the find_package(ICUB) ?

Fine with me 👍

[drdanz]

I opened an issue upstream: https://gitlab.kitware.com/cmake/cmake/-/issues/20972
To be honest, I believe that the only way to solve this is in CMake upstream.

[traversaro]

Based on CMake's upstream response, I am afraid the only viable option is to store and restore the YARP_LIBRARIES value before and after find_dependency(YARP), and then in the long term try to get rid of the YARP_LIBRARIES use.