search

robotology / yarp

YARP - Yet Another Robot Platform
http://www.yarp.it
Other
531 stars 195 forks source link

Add new cmake helper function that brings together yarp_prepare_plugin and yarp_add_plugin #2606

Open traversaro opened 3 years ago

traversaro commented 3 years ago

Is your feature request related to a problem? Please describe.

A common failure mode for people developing YARP plugins is that they write their CMake scripts in a way that yarp_add_plugin is called even if the option ENABLE_<deviceName> or similar option is not set. This error is particularly tricky to debug as it means that a fully functional plugin library is compiled and installed, but the library is missing a symbol defined by yarp_prepare_plugin, and so the plugin can't be loaded (see https://github.com/dic-iit/bipedal-locomotion-framework/issues/335).

Describe the solution you'd like Unfortunately, we can't detect if ENABLE_<deviceName> is set in yarp_add_plugin as the option name can be changed via the OPTION argument of yarp_prepare_plugin. So, to reduce the probability of this happening in the future, I wonder if we could add an option or add a new macro that merges together yarp_prepare_plugin and yarp_add_plugin, that basically just runs yarp_prepare_plugin and if the option is enabled calls yarp_add_plugin. In this way, faulting CMake code that calls target_*** methods on the library created by yarp_add_plugin will early fail at CMake configuration time, instead of silently installing faulty YARP devices.

Describe alternatives you've considered Not sure.

Additional context This already happened several times in the past, but lately in https://github.com/dic-iit/bipedal-locomotion-framework/issues/335 and fixed in https://github.com/dic-iit/bipedal-locomotion-framework/pull/337 .

traversaro commented 3 years ago

fyi @GiulioRomualdi @prashanthr05

drdanz commented 3 years ago

If I understand the issue correctly, the INTERNAL flag handles this... It sets the variable as an internal variable, so that it cannot be modified by the user, and the plugin is always enabled...

drdanz commented 3 years ago

https://github.com/robotology/yarp/blob/f2471e4890ef009f2e3042a46bad251a94f4d2b1/cmake/YarpPlugin.cmake#L222-L224

traversaro commented 3 years ago

Cool, I was not aware of this. @GiulioRomualdi probably for blf's YARP devices it make sense to use INTERNAL, as in any case the enabling/disabling of blf components is already handled by the blf CMake dependency machinery?

traversaro commented 3 years ago

This probably also applies to whole-body-estimators. fyi @HosameldinMohamed @prashanthr05

drdanz commented 3 years ago

I think the INTERNAL flag makes sense for any repository that contains just a few plugins that should never be disabled (for example all the yarp-device-* repositories) and for repositories for which does not make sense to disable the plugins...