Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compile on ROS2 Foxy #153

Merged
merged 7 commits into from
Aug 5, 2020
Merged

Compile on ROS2 Foxy #153

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d42687e
Compile on ROS2 Foxy
henningkayser Jul 18, 2020
be81fea
Suppress assimp CMP0012 policy warning
henningkayser Jul 18, 2020
4e93258
Temporarily disable clang-format
henningkayser Jul 18, 2020
693ac02
Fix CMakeLists issues
henningkayser Jul 18, 2020
e9ae993
Drop eloquent support
henningkayser Jul 18, 2020
b2f47d9
Use console_bridge_vendor dependency wrapper
henningkayser Jul 19, 2020
c054af9
Update Build Status tags in Readme
henningkayser Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@ notifications:
- [email protected]
env:
global:
- ROS_DISTRO=dashing
- ROS_DISTRO=foxy
- ROS_REPO=ros
- UPSTREAM_WORKSPACE=geometric_shapes.repos
- CXXFLAGS="-Wall -Wextra -Wwrite-strings -Wunreachable-code -Wpointer-arith -Wredundant-decls -Wno-unused-parameter"
- WARNINGS_OK=false
matrix:
- TEST="clang-format, ament_lint"
- ROS_DISTRO=dashing
- ROS_DISTRO=eloquent TEST=code-coverage
- TEST="ament_lint" # TODO(henningkayser): re-enable clang-format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang format does not work yet for ROS 2? Is this because there isn't an agreement on version number yet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

- ROS_DISTRO=foxy TEST=code-coverage

matrix:
include:
Expand Down
14 changes: 7 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ if(NOT CMAKE_CONFIGURATION_TYPES AND NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release)
endif()

# TODO(henningkayser): Remove policy fix when assimp 5.1 is available
# Suppress policy warning in assimp (https://github.com/assimp/assimp/pull/2722)
set(CMAKE_POLICY_DEFAULT_CMP0012 NEW)
find_package(ASSIMP QUIET)
if(NOT ASSIMP_FOUND)
find_package(PkgConfig REQUIRED)
Expand All @@ -29,10 +32,11 @@ set(ASSIMP_LIBRARIES "${ASSIMP_ABS_LIBRARIES}")

find_package(rclcpp REQUIRED)
find_package(Boost REQUIRED system filesystem)
find_package(console_bridge_vendor REQUIRED)
find_package(console_bridge REQUIRED)
find_package(eigen3_cmake_module REQUIRED)
find_package(Eigen3 REQUIRED)
find_package(octomap REQUIRED)
find_package(OCTOMAP REQUIRED)
find_package(ament_cmake REQUIRED)
find_package(eigen_stl_containers REQUIRED)
find_package(random_numbers REQUIRED)
Expand Down Expand Up @@ -68,7 +72,7 @@ ament_target_dependencies(${PROJECT_NAME}
geometry_msgs
resource_retriever
console_bridge
octomap
OCTOMAP
ASSIMP
QHULL
)
Expand All @@ -79,15 +83,11 @@ ament_export_dependencies(
Eigen3
eigen3_cmake_module # export Eigen3 headers
Boost
ASSIMP
v4hn marked this conversation as resolved.
Show resolved Hide resolved
random_numbers
eigen_stl_containers
shape_msgs
visualization_msgs
)
target_link_libraries(${PROJECT_NAME}
# resource_retriever doesn't support ament export hook (fixed in 2.1.1)
resource_retriever::resource_retriever
OCTOMAP
)

if(BUILD_TESTING)
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Note: `bodies::ConvexMesh::MeshData` was made implementation-private and is no l

## Build Status

Travis CI: [![Build Status](https://travis-ci.org/ros-planning/geometric_shapes.svg?branch=melodic-devel)](https://travis-ci.org/ros-planning/geometric_shapes)
henningkayser marked this conversation as resolved.
Show resolved Hide resolved
Travis CI: [![Build Status](https://travis-ci.org/ros-planning/geometric_shapes.svg?branch=ros2)](https://travis-ci.org/ros-planning/geometric_shapes)

Devel Job: [![Build Status](http://build.ros.org/buildStatus/icon?job=Msrc_uB__geometric_shapes__ubuntu_bionic__source)](http://build.ros.org/view/Msrc_uB/job/Msrc_uB__geometric_shapes__ubuntu_bionic__source)

Expand Down
2 changes: 1 addition & 1 deletion geometric_shapes.repos
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ repositories:
geometric_shapes:
type: git
url: https://github.com/ros-planning/geometric_shapes
version: dashing-devel
version: ros2
random_numbers:
type: git
url: https://github.com/ros-planning/random_numbers
Expand Down
2 changes: 1 addition & 1 deletion package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't really have much context here. Can you clarify your question a bit more?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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, 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?

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<depend>libqhull</depend>
<depend>octomap</depend>
<depend>random_numbers</depend>
Expand Down