flexible-collision-library / fcl

Flexible Collision Library
https://flexible-collision-library.github.io/
Other
1.35k stars 414 forks source link

Use package format 3 with conditional dependency on catkin. #536

Closed nuclearsandwich closed 3 years ago

nuclearsandwich commented 3 years ago

I'd like to release fcl into ROS 2, starting with ROS 2 Rolling.

catkin isn't used or available in ROS 2. The recommended dependency on catkin for non-ROS packages released on the build farm is covered by the universally injected dependency on the ros_workspace package.


This change is Reviewable

wxmerkt commented 3 years ago

@nuclearsandwich For octomap, there was a community request to add an additional entry to add the library to the ament_index. We also added a conditional dependency on ament_cmake, yet, it wasn't clear if that was strictly necessary. This is what we ended up with: https://github.com/OctoMap/octomap/blob/bdb1c3757e370db93457d710772ab85e8bd44dd4/octomap/package.xml#L16-L23 and https://github.com/OctoMap/octomap/blob/bdb1c3757e370db93457d710772ab85e8bd44dd4/octomap/CMakeLists.txt#L72-L77 Does it make sense to add this correspondingly for FCL as part of this PR or was it completed incorrectly in Octomap? Any insights highly appreciated :-).

sherm1 commented 3 years ago

Thanks for this PR! Can you move the octomap discussion to another PR or issue?

wxmerkt commented 3 years ago

Apologies for being unclear - my question was whether extending on this PR similar to how it's been accomplished for Octomap in the two snippets linked above is desired/required to amend (1) the package.xml touched in this PR to include a conditional dependency on ament_cmake [which based on the original post may be redundant] and (2) to add a registration to the ament_index in the CMakeLists [which may be better as a release patch]. Downstream projects have requested these in the past. If @nuclearsandwich can confirm whether this is necessary (or not), I am happy to make a PR

nuclearsandwich commented 3 years ago

(2) to add a registration to the ament_index in the CMakeLists

Registering fcl in the ament resource index would move it closer to the behavior of "native" ROS 2 packages which simplifies usage for ROS 2 consumers of fcl. However this has also re-sparked a discussion of which packages MUST be registered in an ament index versus which packages MAY be registered. At best registering in the ament index is an expedient and may not ultimately be the recommended path for non-ROS packages however I do not yet have any guidance on an alternative recommended path.

If you're willing to contribute a change similar to https://github.com/OctoMap/octomap/blob/bdb1c3757e370db93457d710772ab85e8bd44dd4/octomap/CMakeLists.txt#L75-L77 that seems like a good "little copying". It would be up to the fcl maintainers to accept it or not.