Add colcon.pkg files to all packages

[mpowelson]

Addresses issue #302 as discussed on rosdep issue 724.

[codecov]

Codecov Report

Merging #303 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
- Coverage   75.64%   75.63%   -0.02%     
==========================================
  Files         167      167              
  Lines        8431     8431              
==========================================
- Hits         6378     6377       -1     
- Misses       2053     2054       +1     

Impacted Files	Coverage Δ
...esseract/tesseract_collision/src/fcl/fcl_utils.cpp	88.99% <0.00%> (-0.48%)	⬇️

[gavanderhoorn]

What's the specific rationale here?

If most (all?) of these packages are ROS-agnostic, and the wrappers are in tesseract_ros and tesseract_ros2, it seems like a strange thing to add these markers here.

IIUC, these are needed when packages contain resources ROS 2 packages should be able to find at runtime. For build dependencies it would seem this is already covered by CMake's find_package(..) et al.

Do all of these packages have plugins or other resources for ROS 2 packages to consume?

[mpowelson]

This is supposed to fix the underlay issue on tesseract_ros2 when tesseract is in the upstream workspace and tesseract_ros2 is in the target workspace (tesseract-robotics/tesseract_ros2#21). Apparently it was temporarily patched in industrial_ci but that change will be reverted eventually. @Levi-Armstrong could give more details. I do agree that it feels like this should be unnecessary.

[Levi-Armstrong]

This is to expose these packages to the ROS2 eco system so they are found by rosdep, ros2 find package library, plugins (Though this is just because we are using ROS2 libraries for finding plugin), etc.. Currently in ROS2 it leverages the AMENT_PREFIX_PATH for finding resources which by default non-ament package do not get added to this environment variable which was the case in ROS1. By adding the environment hooks this exposes these packages to ROS2 eco system.

[gavanderhoorn]

This is to expose these packages to the ROS2 eco system so they are found by rosdep

Ah, that 'bug'/omission.

ros2 find package library, plugins (Though this is just because we are using ROS2 libraries for finding plugin),

Ok, so you do have a need for the runtime side of this.

Currently in ROS2 it leverages the AMENT_PREFIX_PATH for finding resources which by default non-ament package do not get added to this environment variable which was the case in ROS1. By adding the environment hooks this exposes these packages to ROS2 eco system.

Yes, I'd understood that.

I was not aware you were using pluginlib, even in the ROS-agnostic side of Tesseract.

[Levi-Armstrong]

I was not aware you were using pluginlib, even in the ROS-agnostic side of Tesseract.

I just checked and the plugins are in tesseract_ros2, but the library they load are in tesseract_collision so without the hook it cannot be found because tesseract_collision is not in the AMENT_PREFIX_PATH.

[Levi-Armstrong]

@mpowelson With this change we need to now remove the lines from tesseract_collision CMakeLists.txt.

# Create an ament_index resource file to allow ROS2 ament_index_cpp to locate the installed path to this package.
# This is a workaround to let the ROS2 version of pluginlib find tesseract_collision's plugins, since tesseract_collision is a non-ROS CMake package.
# ADDITIONAL REQUIREMENT: The installed path must be added to the AMENT_PREFIX_PATH environment variable at runtime, which is outside the scope of CMakeLists.txt.
file(WRITE ${CMAKE_INSTALL_PREFIX}/share/ament_index/resource_index/packages/${PROJECT_NAME} "")

[gavanderhoorn]

I was not aware you were using pluginlib, even in the ROS-agnostic side of Tesseract.

I just checked and the plugins are in tesseract_ros2, but the library they load are in tesseract_collision so without the hook it cannot be found because tesseract_collision is not in the AMENT_PREFIX_PATH.

I'm somewhat confused.

If it's a regular .so, then I would expect the wrapper which makes it a "ROS plugin" would link in the regular way against the ROS-agnostic library. That would all be regular CMake/make.

For the ROS wrapper-plugin to be found, that would need an index entry. That I understood.

But the plain .so?

[Levi-Armstrong]

@schornakj When dealing with the collision plugin for the ros2 package why did you also need to modify the AMENT_PREFIX_PATH environment variable?

@gavanderhoorn Ah, I believe the issue may be that we are Privately linking against tesseract libraries for the plugins.

add_library(tesseract_collision_bullet_plugin src/plugins/bullet_plugin.cpp)
target_link_libraries(tesseract_collision_bullet_plugin PRIVATE tesseract::tesseract_collision_core tesseract::tesseract_collision_bullet ${catkin_LIBRARIES})
tesseract_target_compile_options(tesseract_collision_bullet_plugin PRIVATE)
target_include_directories(tesseract_collision_bullet_plugin SYSTEM PRIVATE
    ${EIGEN3_INCLUDE_DIRS}
    ${catkin_INCLUDE_DIRS})

add_library(tesseract_collision_fcl_plugin src/plugins/fcl_plugin.cpp)
target_link_libraries(tesseract_collision_fcl_plugin PRIVATE tesseract::tesseract_collision_core tesseract::tesseract_collision_fcl ${catkin_LIBRARIES})
tesseract_target_compile_options(tesseract_collision_fcl_plugin PRIVATE)
target_include_directories(tesseract_collision_fcl_plugin SYSTEM PRIVATE
    ${catkin_INCLUDE_DIRS})

[Levi-Armstrong]

@mpowelson it looks like benchmark is missing from the skip keys for the nightly ci build for xenial.

[gavanderhoorn]

@gavanderhoorn Ah, I believe the issue may be that we are Privately linking against tesseract libraries for the plugins.

Is there a specific reason for that?

[Levi-Armstrong]

Is there a specific reason for that?

No, It should be public.

[schornakj]

@schornakj When dealing with the collision plugin for the ros2 package why did you also need to modify the AMENT_PREFIX_PATH environment variable?

@Levi-Armstrong @gavanderhoorn Sorry for not responding to this especially quickly. As a cross-reference, when I was originally working through this I created an issue on ros/pluginlib.

In Summary:

The ROS2 version of pluginlib's ClassLoader requires that the package containing the plugin library be listed in AMENT_PREFIX_PATH. This is a little different from how it works in the ROS1 version: in that case it uses ROS_PACKAGE_PATH, and we were able to include the ROS-agnostic Tesseract packages on that path.

In More Detail:

The tesseract_ros2 environment monitor node needs to load contact manager plugins at runtime, which it does like this:

discrete_manager_loader_.reset(new DiscreteContactManagerPluginLoader("tesseract_collision",
                                                                      "tesseract_collision::"
                                                                      "DiscreteContactManager"));

When we get to ClassLoader, we have these lines:

try {
  ament_index_cpp::get_package_prefix(package_);
} catch (const ament_index_cpp::PackageNotFoundError & exception) {
  // rethrow as class loader exception, package name is in the error message already.
  throw pluginlib::ClassLoaderException(exception.what());
}

In our case, this tries to call ament_index_cpp::get_package_prefix("tesseract_collision"), which searches the AMENT_PREFIX_PATH environment variable for tesseract_collision. The tesseract_collision package doesn't include any of the ament_cmake package setup functions since it's trying to be ROS-agnostic, so it is not added to AMENT_PREFIX_PATH. This results in an exception being thrown when we try to run this node.

The quickest way to get this working was to modify tesseract_collision/CMakeLists.txt so it would create the right files that would let it be added to AMENT_PREFIX_PATH.

[gavanderhoorn]

The ROS2 version of pluginlib's ClassLoader requires that the package containing the plugin library be listed in AMENT_PREFIX_PATH. This is a little different from how it works in the ROS1 version: in that case it uses ROS_PACKAGE_PATH, and we were able to include the ROS-agnostic Tesseract packages on that path.

My confusion was specifically about the fact that it appeared the PR adds all packages to the resource index.

Afaict only the packages which host resources which must be findable at runtime would need this.

Your comments confirm my understanding.

It's still unclear to me why the rest of the packages need to be on the resource index though.

[Levi-Armstrong]

It's still unclear to me why the rest of the packages need to be on the resource index though.

I do see your point, but is there any harm with having them all available?