search

robotology / wearables

Code moved to https://github.com/robotology/human-dynamics-estimation
https://github.com/robotology/human-dynamics-estimation
BSD 3-Clause "New" or "Revised" License
19 stars 11 forks source link

Add dependencies for IWearLogger in the documentation #142

Closed RiccardoGrieco closed 2 years ago

RiccardoGrieco commented 2 years ago

This PR to enhance the documentation related to IWearLogger.

As for now, it is not built/installed if yarp-telemetry is missing. In order to make users aware of that, I added the info on the README's dependencies section. I also added a CMake warning message for the same purpose.

traversaro commented 2 years ago

Ok for this specific case and this was already the case, but in general it is not a good idea to select/deselect which parts of the build are enabled based on the environment. The problem with this is that if some optional dependency is not found in the environment, you do not realize this until you try to use the part you need. We tipically use <proj>_USES_<deps> exactly for this reason.

traversaro commented 2 years ago

Ok for this specific case and this was already the case, but in general it is not a good idea to select/deselect which parts of the build are enabled based on the environment. The problem with this is that if some optional dependency is not found in the environment, you do not realize this until you try to use the part you need. We tipically use <proj>_USES_<deps> exactly for this reason.

See also https://github.com/stack-of-tasks/pinocchio/issues/1323#issuecomment-709120032 for a related comment, in particular the video link https://www.youtube.com/watch?v=sBP17HQAQjk&t=653s .

traversaro commented 2 years ago

For me we can merge.

RiccardoGrieco commented 2 years ago

Thanks @traversaro @lrapetti for the review.

For the time being let's keep these changes as this affects only users who manually build the repository, since in the superbuild yarp-telemetry is automatically built with wearables.

Proceeding with the merge.