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.

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

Update CI to ROS2 #37

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
70 changes: 25 additions & 45 deletions .docker/ci/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,50 +1,30 @@
# moveit/moveit:melodic-ci
Copy link
Member

Choose a reason for hiding this comment

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

We should keep as much of the comments and structure as possible

# Sets up a base image to use for running Continuous Integration on Travis
FROM ros:crystal-ros-base-bionic
Copy link

Choose a reason for hiding this comment

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

What do you think if we use this a base and after that install the following: apt install -y ros-crystal-desktop It includes some of the dependencies. I've try it out locally and seems that is working.

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

@henningkayser henningkayser Mar 12, 2019

Choose a reason for hiding this comment

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

Yes, we could use the desktop docker, but this would probably include a lot of unnecessary packages as well. I think installing packages by hand is fine, just use rosdep where possible.

Copy link

Choose a reason for hiding this comment

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

+1


FROM ros:melodic-ros-base
MAINTAINER Dave Coleman [email protected]
MAINTAINER Lander Usategui <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this should change, @davetcoleman ?

Copy link

Choose a reason for hiding this comment

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

No problem with this, I'll keep Dave as maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Thanks @ruffsl!

Copy link
Member

Choose a reason for hiding this comment

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

i do not need to be a maintainer of this anymore


ENV TERM xterm
Copy link
Member

Choose a reason for hiding this comment

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

We need xterm for ANSI colors and text folding

ENV ROS_DISTRO crystal
WORKDIR /ros2_ws

# Setup catkin workspace
ENV CATKIN_WS=/root/ws_moveit
Copy link
Member

Choose a reason for hiding this comment

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

Even though catkin not used anymore, we need this variable in test and build scripts.
Maybe just rename it to OVERLAY_WS.

WORKDIR $CATKIN_WS

# Commands are combined in single RUN statement with "apt/lists" folder removal to reduce image size
# https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#minimize-the-number-of-layers
RUN \
mkdir src && \
cd src && \
#
# Download moveit source, so that we can get necessary dependencies
wstool init --shallow . https://raw.githubusercontent.com/ros-planning/moveit/${ROS_DISTRO}-devel/moveit.rosinstall && \
#
# Update apt package list as previous containers clear the cache
apt-get -qq update && \
apt-get -qq dist-upgrade && \
#
# Install some base dependencies
apt-get -qq install -y \
# Some source builds require a package.xml be downloaded via wget from an external location
apt-get update && apt-get install -y \
python3-colcon-common-extensions \
libboost-all-dev \
Copy link
Member

Choose a reason for hiding this comment

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

These should be installed with rosdep eventually so maybe add a TODO for that

Copy link
Member

Choose a reason for hiding this comment

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

Also, are these really all packages that are required to build moveit_core?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these should come with the ROS crystal image.

Copy link

Choose a reason for hiding this comment

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

Not really, we've extra dependencies for the rest of the modules.

Copy link
Member

Choose a reason for hiding this comment

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

For moveit1, we only have to manually install 3 packages:

 sudo apt-get install python-wstool python-catkin-tools clang-format-3.9

And clang-format is actually optional. We should be able to do the same for ros2

https://moveit.ros.org/install/source/

libglew-dev \
freeglut3-dev \
pkg-config \
libfcl-dev \
libassimp-dev \
libqhull-dev \
ros-$ROS_DISTRO-action-msgs \
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be rosdeps.

ros-$ROS_DISTRO-resource-retriever \
ros-$ROS_DISTRO-image-transport \
python3-vcstool \
libopencv-dev \
wget \
# Required for rosdep command
sudo \
# Preferred build tools
python-catkin-tools \
clang clang-format-3.9 clang-tidy clang-tools \
ccache && \
#
# Download all dependencies of MoveIt!
rosdep update && \
rosdep install -y --from-paths . --ignore-src --rosdistro ${ROS_DISTRO} --as-root=apt:false && \
#
# Remove the source code from this container
# TODO: in the future we may want to keep this here for further optimization of later containers
cd .. && \
rm -rf src/ && \
#
# Clear apt-cache to reduce image size
rm -rf /var/lib/apt/lists/*

# Continous Integration Setting
ENV IN_DOCKER 1
Copy link
Member

Choose a reason for hiding this comment

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

This is necessary for moveit_ci

&& rm -rf /var/lib/apt/lists/* \
# Setup ROS2 workspace
&& mkdir -p /ros2_ws/src && cd /ros2_ws \
&& wget https://raw.githubusercontent.com/AcutronicRobotics/moveit2/master/external-repos.repos \
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed to ros-planning/moveit2

&& vcs import src < external-repos.repos
COPY ./docker-entrypoint.sh /
ENTRYPOINT ["/docker-entrypoint.sh"]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using this endpoint we should use the build script from the moveit_ci package. @mlautman is in progress on porting it to ros2 moveit/moveit_ci#50.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, moveit_ci has some really nice features that have been developed over time that will really benefit MoveIt2

8 changes: 8 additions & 0 deletions .docker/ci/docker-entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/bin/bash

set -e

/bin/bash -c "find /opt/ros/crystal/ -name tf2_eigen | xargs rm -rf && source /opt/ros/crystal/setup.bash \
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing tf2_eigen from the source install? This needs good documentation

Copy link

Choose a reason for hiding this comment

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

The main reason behind this was this: AcutronicRobotics#19 (comment)

&& touch /ros2_ws/src/image_common/camera_calibration_parsers/COLCON_IGNORE \
Copy link
Contributor

Choose a reason for hiding this comment

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

camera_calibration_parsers and camera_info_manager can be skipped with --packages-skip camera_info_manager camera_calibration_parsers

Copy link

Choose a reason for hiding this comment

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

I prefer to have the de colcon command as clean as we can, but if you prefer to skip the packages using this way, I'll change it.

&& touch /ros2_ws/src/image_common/camera_info_manager/COLCON_IGNORE \
&& colcon build --merge-install"
52 changes: 18 additions & 34 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,43 +1,27 @@
# This config file for Travis CI utilizes https://github.com/ros-planning/moveit_ci/ package.
sudo: required
dist: xenial # distro used by Travis, moveit_ci uses the docker image's distro
services:
- docker
language: cpp
cache: ccache
compiler: gcc

notifications:
email:
recipients:
- [email protected]
env:
Copy link
Member

Choose a reason for hiding this comment

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

All these settings are necessary for different tests in moveit_ci so they should be kept or updated to crystal.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

global:
- MOVEIT_CI_TRAVIS_TIMEOUT=85 # Travis grants us 90 min, but we add a safety margin of 5 min
- ROS_DISTRO=melodic
- ROS_REPO=ros
- UPSTREAM_WORKSPACE=moveit.rosinstall
- TEST_BLACKLIST=moveit_ros_perception # mesh_filter_test fails due to broken Mesa OpenGL
- WARNINGS_OK=false
matrix:
- TEST="clang-format catkin_lint"
Copy link
Member

Choose a reason for hiding this comment

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

only remove catkin_lint here

- TEST=clang-tidy-fix
- ROS_DISTRO=melodic
Copy link
Member

Choose a reason for hiding this comment

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

Just change to crystal and remove kinetic. We still need to discuss if we want to test multiple ros2 versions yet

- ROS_DISTRO=kinetic
- [email protected]
- [email protected]

matrix: # Add a separate config to the matrix, using clang as compiler
matrix:
include:
# add a config to the matrix using clang as compiler and also test ikfast plugin creation
- compiler: clang
env: ROS_REPO=ros-shadow-fixed
BEFORE_DOCKER_SCRIPT="source moveit_kinematics/test/test_ikfast_plugins.sh"

fast_finish: true # finish, even if allow-failure-tests still running
allow_failures:
- env: TEST=clang-tidy-fix # need to fix clang-tidy issues
- os: linux
dist: bionic
services:
- docker
sudo:
- required
- os: osx
osx_image: xcode10.1

before_script:
- git clone -q --depth=1 https://github.com/ros-planning/moveit_ci.git .moveit_ci
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then bash ./osx-dependencies.sh; fi
Copy link
Member

Choose a reason for hiding this comment

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

OSX support should be implemented in an extra docker file that clones the repo and installs dependencies.
I'm not sure if we want to run the whole moveit_ci stack for osx so I guess here it would be ok to use ENTRYPOINT for compiling.

Copy link

Choose a reason for hiding this comment

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

Thank you for your review! AFAIK we cannot build an OS X image on Docker. What do you mean exactly here?

Copy link
Member

Choose a reason for hiding this comment

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

You're right about the docker image. Other than running a VM inside docker there is probably no way to do this. My point is that we should try to use the same setup and build methods accross different platforms. The question is if we want to implement osx support in moveit_ci

Copy link
Member

Choose a reason for hiding this comment

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

i think one issue here is that two very different features are being combined in the same PR: migrating to ROS2 and also, adding support for OSX. separating these would make discussion much easier

- mkdir /tmp/moveit2
- mv * /tmp/moveit2
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then cd .docker/ci; fi

script:
- .moveit_ci/travis.sh
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then docker build -t acutronicrobotics/moveit2 .; fi
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then docker run -v /tmp/moveit2:/ros2_ws/src/moveit2 acutronicrobotics/moveit2; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then bash /tmp/moveit2/osx-compile.sh; fi
61 changes: 61 additions & 0 deletions external-repos.repos
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
repositories:
Copy link
Member

Choose a reason for hiding this comment

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

Why is moveit2 not included here? This way travis just compiles the dependencies only.
Also we should rename this file to moveit2.repos for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to separate temporary dependencies to AcutronicRobotics and those that are not released yet somehow. Either just with comments or in a separate .repos file.

Copy link

Choose a reason for hiding this comment

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

Why is moveit2 not included here? This way travis just compiles the dependencies only.
Also we should rename this file to moveit2.repos for consistency.

+1 for rename the file. I use in this way to mount the repo as a volume on Docker when we build it. This way we can build the code of the current branch. But, I can set the corresponding branch in moveit2.repos for each branch.

Copy link
Member

Choose a reason for hiding this comment

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

I totally see the point in doing it this way. moveit_ci already handles the corresponding branch inside travis.sh anyway. I think the original reason why the parent repo was included in .rosinstall files is that it makes it easier to setup workspaces. @rhaschke any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fully sure what you are referring to exactly.
But, yes MoveIt's .rosinstall file comprises all repositories required to build MoveIt from source, i.e. also including MoveIt itself. travis.sh here takes care that the to-be-tested repo is removed from the .rosinstall file before initializing the workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LanderU Is there a reason to switch from .rosinstall to .repos? A good deal of the MoveIt infrastructure assumes the use of wstool with .rosinstall files. We can migrate over to vcs but we would probably want to have good reasons for doing so.

Copy link

Choose a reason for hiding this comment

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

We're using .repos because ROS2 is using this kind of file: https://github.com/ros2/ros2/blob/master/ros2.repos

Copy link
Member

Choose a reason for hiding this comment

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

Does wstool + .rosinstall files not also work with ROS2? I think keeping the moveit1 approach, at least for now, will reduce a lot of headache. I think focusing on the minimum changes for ROS2 support would be the best first approach

Copy link
Member

Choose a reason for hiding this comment

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

wstool still works since it's pretty much unrelated to any ROS/ROS2 functionality. vcstools is a new alternative to wstool that is supported by OSRF and comes with some improved features (vcstools/wstool#19 (comment)) which we don't need for migration. However, the import function of vcstools supports .rosinstall files (see README) so we should be able to use it as a drop-in replacement.

moveit_msgs:
type: git
url: https://github.com/AcutronicRobotics/moveit_msgs
version: ros2
geometry2:
type: git
url: https://github.com/AcutronicRobotics/geometry2
version: master
geometric_shapes:
type: git
url: https://github.com/AcutronicRobotics/geometric_shapes
version: ros2
eigen_stl_containers:
type: git
url: https://github.com/AcutronicRobotics/eigen_stl_containers
version: ros2
object_recognition_msgs:
type: git
url: https://github.com/AcutronicRobotics/object_recognition_msgs
version: master
octomap_msgs:
type: git
url: https://github.com/AcutronicRobotics/octomap_msgs
version: ros2
random_numbers:
type: git
url: https://github.com/AcutronicRobotics/random_numbers
version: ros2
srdfdom:
type: git
url: https://github.com/AcutronicRobotics/srdfdom
version: ros2
urdf_parser_py:
type: git
url: https://github.com/AcutronicRobotics/urdf_parser_py
version: ros2
octomap:
type: git
url: https://github.com/AcutronicRobotics/octomap
version: ros2
moveit_resources:
type: git
url: https://github.com/AcutronicRobotics/moveit_resources
version: master
console_bridge_vendor:
type: git
url: https://github.com/ros2/console_bridge_vendor
version: crystal
ros1_bridge:
type: git
url: https://github.com/ros2/ros1_bridge
version: master
image_common:
type: git
url: https://github.com/ros-perception/image_common
version: ros2
angles:
type: git
url: https://github.com/ros/angles
version: ros2
22 changes: 22 additions & 0 deletions osx-compile.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/bin/bash

#set -x #Debug
set -e #exit on failure

#Prepare for ros2
cd /tmp
wget https://github.com/ros2/ros2/releases/download/release-crystal-20190214/ros2-crystal-20190214-macos-amd64.tar.bz2
tar jxf ros2-crystal-20190214-macos-amd64.tar.bz2
# Remove tf2_eigen
find ros2-osx/ -name tf2_eigen | xargs rm -rf

source ros2-osx/setup.bash
mkdir -p /tmp/ros2_ws/src
cp -r /tmp/moveit2 /tmp/ros2_ws/src
cd /tmp/ros2_ws && wget https://raw.githubusercontent.com/AcutronicRobotics/moveit2/master/external-repos.repos
vcs import src < external-repos.repos
export OPENSSL_ROOT_DIR="/usr/local/opt/openssl"
#Ignore packages
touch src/image_common/camera_calibration_parsers/COLCON_IGNORE
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on why camera_calibration_parsers shouldn't be built in a comment? If this is an OSX issue then we will want to know that we can delete this line when the package is fully ported

Copy link

Choose a reason for hiding this comment

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

Only because for now is not needed, and because most of the packages on this repo have COLCON_IGNORE. We can keep this as is till have correctly ported to ROS2.

touch src/image_common/camera_info_manager/COLCON_IGNORE
colcon build --merge-install
9 changes: 9 additions & 0 deletions osx-dependencies.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/bin/bash

#FROM https://index.ros.org/doc/ros2/Installation/OSX-Development-Setup/

brew doctor
brew install cppcheck pcre poco tinyxml openssl asio tinyxml2 log4cxx assimp qhull boost fcl freeglut glew
python3 -m pip install argcomplete catkin_pkg colcon-common-extensions coverage empy flake8 flake8-blind-except \
flake8-builtins flake8-class-newline flake8-comprehensions flake8-deprecated flake8-docstrings flake8-import-order flake8-quotes \
git+https://github.com/lark-parser/[email protected] mock nose pep8 pydocstyle pyparsing setuptools vcstool