Add Eigen3 link library in WholeBodyControllers component

[valentino-razza]

Fix Eigen include error:

In file included from /home/valentino/work/robotology-superbuild/robotology/walking-controllers/src/WholeBodyControllers/src/InverseKinematics.cpp:21:0:
/home/valentino/work/robotology-superbuild/build/install/include/iDynTree/Core/EigenHelpers.h:16:10: fatal error: Eigen/Dense: No such file or directory
 #include <Eigen/Dense>
          ^~~~~~~~~~~~~
compilation terminated.

cc @CarlottaSartore

[traversaro]

@GiulioRomualdi @S-Dafarra for context, @valentino-razza need this as it was using an old version of osqp-eigen that did not included gbionics/osqp-eigen#62 .

[S-Dafarra]

@GiulioRomualdi @S-Dafarra for context, @valentino-razza need this as it was using an old version of osqp-eigen that did not included robotology/osqp-eigen#62 .

Ok, clear. In any case, it does no harm. So we may merge this anyway. @GiulioRomualdi what do you think?

[GiulioRomualdi]

So in theory we do not need it Right?
Is there a reason why do you need a specific version of osqp-eigen?

[GiulioRomualdi]

OK let me understand:
gbionics/osqp-eigen#62 just hides the problem right? This PR is required anyway Also because InverseKinematics.cpp does not use osqp

[traversaro]

OK let me understand:
robotology/osqp-eigen#62 just hides the problem right? This PR is required anyway Also because InverseKinematics.cpp does not use osqp

That is correct. As when including file you should IWYU (include what you use), when linking you should always LWYU (link waht you use), without ever relying on transitive linking, as that can change in the future.

[GiulioRomualdi]

That is correct. As when including file you should IWYU (include what you use), when linking you should always LWYU (link waht you use), without ever relying on transitive linking, as that can change in the future.

I agree

Before merging I ask @valentino-razza to update the changelog adding a line in the unreleased section

[nunoguedelha]

Thanks for the fix @valentino-razza . I was experiencing the same problem on the component iDynTreeUtilities.

Maybe on your environment, the issue was hidden because you could build previously with the workaround and the install folder keeps old objects.

In any case it would make sense to propagate the fix to iDynTreeUtilities.

In addition, as per gbionics/whole-body-estimators#68, shouldn't we add find_package(Eigen3 REQUIRED) to the CMakeLists.txtat the root of the repowalking_controllers`? @S-Dafarra ?

What about the keywords PUBLIC, PRIVATE used in the command target_link_libraries? Aren't those usually required?

[nunoguedelha]

@GiulioRomualdi , I saw that the ongoing fix is on top of master=v0.4.1. are there any particular dependencies between v0.3.0 and v0.4.1, regarding the other repos of the superbuild?

[S-Dafarra]

n addition, as per robotology/whole-body-estimators#68, shouldn't we add find_package(Eigen3 REQUIRED) to the CMakeLists.txtat the root of the repowalking_controllers`? @S-Dafarra ?

It is here: https://github.com/robotology/walking-controllers/blob/master/cmake/WalkingControllersFindDependencies.cmake#L134

[nunoguedelha]

@GiulioRomualdi , I saw that the ongoing fix is on top of master=v0.4.1. are there any particular dependencies between v0.3.0 and v0.4.1, regarding the other repos of the superbuild?

Sorry @GiulioRomualdi , disregard this comment, I was missing some information.

In any case it would make sense to propagate the fix to iDynTreeUtilities.

@valentino-razza , I just saw your comments in the "other project", and that this is already in v0.4.1.

@GiulioRomualdi, my only remaining question is about this:

What about the keywords PUBLIC, PRIVATE used in the command target_link_libraries? Aren't those usually required?

[S-Dafarra]

@GiulioRomualdi, my only remaining question is about this:

What about the keywords PUBLIC, PRIVATE used in the command target_link_libraries? Aren't those usually required?

They are supposed to be there, aren't they? In any case, they are required.

[nunoguedelha]

PRIVATE is missing in the Eigen3 entry, but walking-controllers is building anyway.

Side note

It's probably not a big deal in this case, but for limiting the propagation of useless dependencies, it's recommended to use PRIVATE when it applies right?

My only knowledge on the topic comes from:
https://cmake.org/pipermail/cmake/2016-May/063400.html
https://leimao.github.io/blog/CMake-Public-Private-Interface/

As per what is explained in https://cmake.org/pipermail/cmake/2016-May/063400.html, I was expecting the entries in target_link_libraries to have respective entries in target_include_directories for adding the required includes, but I don't see this in the CMakeLists.txt of whole-body-estimators and walking-controllers so I don't really understand how it works.

[traversaro]

PRIVATE is missing in the Eigen3 entry, but walking-controllers is building anyway.

target_link_libraries also support not specifying the INTERFACE/PUBLIC/PRIVATE specifier, see https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-both-a-target-and-its-dependents . If you don't specify it, it is more and less PUBLIC by default, but it better to always specify it as mixing the two ways of specifying them is not supported.

[traversaro]

PRIVATE is missing in the Eigen3 entry, but walking-controllers is building anyway.

target_link_libraries also support not specifying the INTERFACE/PUBLIC/PRIVATE specifier, see https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-both-a-target-and-its-dependents . If you don't specify it, it is more and less PUBLIC by default, but it better to always specify it as mixing the two ways of specifying them is not supported.

Sorry, I did not saw the code. If you have target_link_libraries(<lib> PUBLIC lib1 lib2 lib3) then PUBLIC is intended for all the libraries linked, so lib1, lib2 and lib3 are all linked with PUBLIC , and in this PR Eigen3 is linked with public.

[traversaro]

It's probably not a big deal in this case, but for limiting the propagation of useless dependencies, it's recommended to use PRIVATE when it applies right?

Yes, typically that is the case, but in this case as Eigen3 is already included in public headers via the OsqpEigen.h use, I think it is fine to just use it as PUBLIC .

As per what is explained in https://cmake.org/pipermail/cmake/2016-May/063400.html, I was expecting the entries in target_link_libraries to have respective entries in target_include_directories for adding the required includes, but I don't see this in the CMakeLists.txt of whole-body-estimators and walking-controllers so I don't really understand how it works.

If you do:

target_link_libraries(lib PRIVATE lib1)

the correct include directories that are set for lib1 (i.e. that are contained in its INTERFACE_INCLUDE_DIRECTORIES are automatically set for lib thanks to the so-called "transitive usage requirements", see https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#transitive-usage-requirements . Thanks to this, you don't need to have target_include_directories(lib PRIVATE ${lib1_INCLUDE_DIRS}) as it was necessary in the past.

[valentino-razza]

I've committed changes to CHANGELOG.md