Update CI to ROS2 #37

vmayoral · 2019-03-10T16:36:30Z

Submit changes to enable CI. @LanderU, feel free to add anything in case I missed something.

@LanderU

Authored by @LanderU

henningkayser

Thanks for setting this up! As mentioned in some comments below Moveit CI runs a well elaborated selection of build and test cases. We really want to maintain that functionality, which is why @mlautman is currently working on migrating moveit_ci.
Docker files and travis settings are already configured and fine tuned for moveit_ci so we just want to modify as much as necessary. This also includes keeping the existing structure and comments.
I'm currently working on setting up docker with moveit_ci myself (here) to get everything running, so we could either apply the changes to this PR or I create a new one including your contributions.

henningkayser · 2019-03-11T18:13:54Z

.docker/ci/Dockerfile

@@ -1,50 +1,30 @@
-# moveit/moveit:melodic-ci


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

henningkayser · 2019-03-11T18:21:56Z

.docker/ci/Dockerfile


-ENV TERM xterm


We need xterm for ANSI colors and text folding

henningkayser · 2019-03-11T18:25:13Z

.docker/ci/Dockerfile

+        && 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 \


This should be changed to ros-planning/moveit2

henningkayser · 2019-03-11T18:27:39Z

.docker/ci/Dockerfile


-# Setup catkin workspace
-ENV CATKIN_WS=/root/ws_moveit


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

henningkayser · 2019-03-11T18:30:58Z

.docker/ci/Dockerfile

+    && wget https://raw.githubusercontent.com/AcutronicRobotics/moveit2/master/external-repos.repos \
+    && vcs import src < external-repos.repos
+COPY ./docker-entrypoint.sh /
+ENTRYPOINT ["/docker-entrypoint.sh"]


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.

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

henningkayser · 2019-03-11T18:41:15Z

.travis.yml

-  matrix:
-    - TEST="clang-format catkin_lint"
-    - TEST=clang-tidy-fix
-    - ROS_DISTRO=melodic


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

henningkayser · 2019-03-11T18:55:58Z

.travis.yml


 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


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.

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

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

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

henningkayser · 2019-03-11T18:57:19Z

.docker/ci/Dockerfile

-        # 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 \


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

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

Some of these should come with the ROS crystal image.

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

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/

henningkayser · 2019-03-11T19:13:33Z

external-repos.repos

@@ -0,0 +1,61 @@
+repositories:


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.

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.

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.

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?

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.

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

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

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

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.

henningkayser · 2019-03-11T19:17:46Z

.docker/ci/Dockerfile


-FROM ros:melodic-ros-base
-MAINTAINER Dave Coleman dave@picknik.ai
+MAINTAINER Lander Usategui  <lander@erlerobotics.com>


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

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

FYI that field is deprecated:
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated

Thanks @ruffsl!

i do not need to be a maintainer of this anymore

LanderU · 2019-03-12T09:08:50Z

.docker/ci/Dockerfile

@@ -1,50 +1,30 @@
-# moveit/moveit:melodic-ci
-# Sets up a base image to use for running Continuous Integration on Travis
+FROM ros:crystal-ros-base-bionic


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.

https://github.com/osrf/docker_images/blob/eb35e27e8a0842c8fb493bbdef37b314f931aa59/ros/crystal/ubuntu/bionic/desktop/Dockerfile#L5-L8

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.

mlautman · 2019-03-12T19:12:07Z

.docker/ci/Dockerfile

+        libfcl-dev \
+        libassimp-dev \
+        libqhull-dev \
+        ros-$ROS_DISTRO-action-msgs \


These should be rosdeps.

mlautman · 2019-03-12T19:28:01Z

osx-compile.sh

+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


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

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.

mlautman · 2019-03-12T19:34:56Z

.docker/ci/docker-entrypoint.sh

+set -e
+
+/bin/bash -c "find /opt/ros/crystal/ -name tf2_eigen | xargs rm -rf && source /opt/ros/crystal/setup.bash \
+	      && touch /ros2_ws/src/image_common/camera_calibration_parsers/COLCON_IGNORE \


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

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.

mlautman · 2019-03-12T19:36:31Z

.docker/ci/docker-entrypoint.sh

+
+set -e
+
+/bin/bash -c "find /opt/ros/crystal/ -name tf2_eigen | xargs rm -rf && source /opt/ros/crystal/setup.bash \


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

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

mlautman · 2019-03-13T01:17:18Z

@vmayoral @LanderU I am having a little trouble getting the AcutronicRobotics master branch to build on my machine. It seems that rosdep install doesn't work yet so I went through the dependencies and fixed all the issues that I found along the way:

AcutronicRobotics/urdf_parser_py#1
moveit/srdfdom#48
AcutronicRobotics/octomap#1
AcutronicRobotics/octomap_msgs#7
AcutronicRobotics/moveit_msgs#2
AcutronicRobotics/object_recognition_msgs#5

Now that all of that works on my machine, I am trying to get colcon build --packages-up-to moveit_core to run successfully.

Are there any feature branches that I could be using to make this process go a little smoother?

davetcoleman · 2019-03-19T18:56:57Z

ping @LanderU do you have any advice on how to get the Acutronics master branch compiling?

vmayoral · 2019-03-30T09:04:09Z

I'm closing this on behalf of the CI work @mlautman has been leading.

Update CI to ROS2

fa82fc5

Authored by @LanderU

henningkayser requested changes Mar 11, 2019

View reviewed changes

LanderU reviewed Mar 12, 2019

View reviewed changes

mlautman reviewed Mar 12, 2019

View reviewed changes

.docker/ci/Dockerfile

libfcl-dev \

libassimp-dev \

libqhull-dev \

ros-$ROS_DISTRO-action-msgs \

Copy link

Contributor

mlautman Mar 12, 2019

Choose a reason for hiding this comment

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

These should be rosdeps.

mlautman reviewed Mar 12, 2019

View reviewed changes

mlautman mentioned this pull request Mar 15, 2019

Moveit2 ci #46

Merged

7 tasks

vmayoral closed this Mar 30, 2019


		set -e

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

Update CI to ROS2 #37

Update CI to ROS2 #37

Conversation

vmayoral commented Mar 10, 2019

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningkayser Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman commented Mar 13, 2019

davetcoleman commented Mar 19, 2019

vmayoral commented Mar 30, 2019

henningkayser Mar 12, 2019 •

edited

Loading