Compile on ROS2 Foxy #153
Compile on ROS2 Foxy #153
Conversation
Codecov Report
@@ Coverage Diff @@
## ros2 #153 +/- ##
==========================================
- Coverage 55.78% 55.62% -0.17%
==========================================
Files 12 12
Lines 1719 1697 -22
==========================================
- Hits 959 944 -15
+ Misses 760 753 -7
Continue to review full report at Codecov.
|
* Remove ASSIMP export dependency * Fix OCTOMAP dependency * Remove redundant linking of resource_retriever
| @@ -28,7 +28,7 @@ | |||
| <depend>rclcpp</depend> | |||
| <depend>boost</depend> | |||
| <depend>eigen_stl_containers</depend> | |||
| <depend>libconsole-bridge-dev</depend> | |||
| <depend>console_bridge_vendor</depend> | |||
There was a problem hiding this comment.
First they make quite a fuss about having console-bridge as a ROS-agnostic library and now people rely on a bad, i.e. using externalproject in cmake instead of adapting the package, wrapper to build it in their workspaces???
Is this really required and the official way to do so?
There was a problem hiding this comment.
I'm also a bit confused about this. What I noticed is that you might end up with different console_bridge versions so you should use one or the other dependency consistently. Since the vendor package is used by several upstream dependencies of MoveIt (i.e. urdfdom) I adapted it in all ros-planning packages. I'm not aware of any "official" way, though.
There was a problem hiding this comment.
@ahcorde @clalancette could one of you enlighten us on why this is needed and why you did not adapt the upstream repository (which is the recommended way to package ros-agnostic packages at least in ROS) but go for the ExternalProject approach?
@henningkayser This question is independent of my approval here though.
There was a problem hiding this comment.
Sorry, I don't really have much context here. Can you clarify your question a bit more?
There was a problem hiding this comment.
@clalancette thanks for asking back. My question is whether and why all ROS2 projects should switch to console_bridge_vendor (packaged in ROS distributions) in favor of libconsole-bridge-dev (packaged in your linux distribution).
The second part of the question is why console_bridge_vendor relies on cmake's ExternalProject (downloading the actual code tarball at build-time) instead of following the ROS-established release procedure for third-party packages.
I highlighted you because you are involved with the console_bridge_vendor package according to the commit log. :)
There was a problem hiding this comment.
Just to add, urdfdom is depending on both console_bridge_vendor and libconsole-bridge-dev (https://github.com/ros2/urdfdom/blob/ros2/package.xml#L15). Isn't the dev package redundant or should it be added here as welll? btw, I just noticed that urdfdom still depends on libconsole-bridge0.4 while console_bridge_vendor now provides 0.5. This seems to cause some conflicts with the library versions when using the debians (https://travis-ci.com/github/ros-planning/moveit2/jobs/368602160#L993) so we might need a new release for MoveIt.
There was a problem hiding this comment.
@clalancette thanks for asking back. My question is whether and why all ROS2 projects should switch to
console_bridge_vendor(packaged in ROS distributions) in favor of libconsole-bridge-dev (packaged in your linux distribution).
I wasn't involved in the original vendoring. However, I believe the original reason we ended up vendoring console_bridge was for support on macOS and Windows. I believe that there is now a macOS brew package for it, so that reason may have gone away there, but it is still valid for Windows. We are also vendoring it because we are currently depending on a newer version than what is available in the Ubuntu repositories (we use 1.0.1, while Ubuntu is only providing 0.4).
Just to add,
urdfdomis depending on bothconsole_bridge_vendorandlibconsole-bridge-dev(https://github.com/ros2/urdfdom/blob/ros2/package.xml#L15). Isn't the dev package redundant or should it be added here as welll?
My understanding is that you need both, but I don't have a good explanation of why. Maybe @dirk-thomas can comment on why https://github.com/ros2/urdfdom/blob/ros2/package.xml depends on both.
There was a problem hiding this comment.
My understanding is that you need both, but I don't have a good explanation of why.
Since libconsole-bridge-dev is a run dependency of console_bridge_vendor I don't see why you would need to declare it in addition to depending on console_bridge_vendor.
* switch to *.com for Travis CI * Temporarily comment ROS1 Devel/Debian Job tags
| - TEST="clang-format, ament_lint" | ||
| - ROS_DISTRO=dashing | ||
| - ROS_DISTRO=eloquent TEST=code-coverage | ||
| - TEST="ament_lint" # TODO(henningkayser): re-enable clang-format |
There was a problem hiding this comment.
clang format does not work yet for ROS 2? Is this because there isn't an agreement on version number yet?
There was a problem hiding this comment.
It's not about the version number, we need a new configuration that should be applied to all repos. clang-format-3.9 is not available on Ubuntu 20.04 so I disabled it in CI. We could temporarily run clang-format-3.9 on the eloquent image (like moveit/moveit2@7aea7a7)
This branch is in preparation of the foxy release including following changes:
I think we can remove dashing-devel after merging this as we would only support Foxy just like MoveIt 2.