Adapt moveit_ci for moveit2 development #56

vmayoral · 2019-04-07T18:11:51Z

This PR aims to align with the development happening at https://github.com/acutronicrobotics/moveit2 while fixes several of the problems that the existing infrastructure has when enabling any of the moveit2 submodules. Roughly:

current moveit_ci (ros2 branch) does not take in consideration several dependencies required to build from source moveit2
Dockerfile (and docker images) used in the hub seem to be missing relevant dependencies, an attempt has been made to manually install what's required but decided instead to build new images from ground-up. Available at https://github.com/AcutronicRobotics/moveit2/blob/master/.docker/ci/Dockerfile. (note that this images build from a simple bionic container https://github.com/AcutronicRobotics/moveit2/blob/master/.docker/ci/Dockerfile#L1 instead of from the Open Robotics images).
- Images in the hub to accelerate the process available at https://cloud.docker.com/repository/docker/acutronicrobotics/moveit2/general
Includes support for OS X. Current moveit_ci required changes for launching from OS X (which still uses the Docker container)
Some ROS packages aren't fully ported and need to be COLCON_IGNORED in the workspace
Disable warnings as errors

ros2 branch of moveit_ci wasn't able to build moveit_core and other moveit2 packages. Summary of fixes: - Changed docker image to one with all dependencies - Made use of vcstools instead - Use common ros2.repos instead - modified scripts to enable local testing in OS X

…into ros2

This ensures enviromental variables are in place and allows to build moveit_core and other moveit packages

…into ros2

Add: export ROS_REPO=acutronicrobotics

…into ros2

Added --merge-install on colcon test command

…into ros2

mlautman · 2019-04-08T21:12:36Z

README.md

@@ -109,18 +109,18 @@ It's also possible to run the moveit\_ci script locally, without Travis. We demo
 First clone the repo you want to test:

    cd /tmp/travis   # any working directory will do
-    git clone https://github.com/ros-planning/moveit2
+    git clone https://github.com/acutronicrobotics/moveit2


Let's keep the fork minimal. Rather than pointing the moveit_ci repo to the acutronics fork, we should instead work to migrate the fixes from the Acutronics fork to the main moveit2 repo.

From what I see, the Acutronics Dockerfile manually installs dependencies rather than using rosdep. This isn't a clean way of getting dependencies. Better would be to modify the package.xml's accordingly, and adding missing dependencies to https://github.com/ros/rosdistro/ as needed.

mlautman · 2019-04-08T21:13:09Z

README.md

+First clone the repo you want to test:
+
+    cd /tmp/travis   # any working directory will do
+    git clone https://github.com/acutronicrobotics/moveit2


the ros-planning repo should not point to the acutronics fork

mlautman · 2019-04-08T21:14:10Z

README.md

+    export CC=gcc
+    export CXX=g++
+    export UPSTREAM_WORKSPACE=moveit.rosinstall
+    export TRAVIS_OS_NAME=osx


newline after code block

mlautman · 2019-04-08T21:14:43Z

travis.sh

@@ -27,7 +27,8 @@ COLCON_EVENT_HANDLING="--event-handlers desktop_notification- status-"
 function run_script() {
   local script
   eval "script=\$$1"  # fetch value of variable passed in $1 (double indirection)
-   if [ "${script// }" != "" ]; then  # only run when non-empty
+   if [ ! -z "${script}" ];then
+   #if [ "${script// }" != "" ]; then  # only run when non-empty


delete out commented code

Why changing this code? Probably the cleaning of $script was for a reason.

IMHO this way to check if variable is empty is easiest to understand. But, if you're not happy with the change I can revert it.

mlautman · 2019-04-08T21:15:21Z

travis.sh

       ros) export DOCKER_IMAGE=moveit/moveit2:$ROS_DISTRO-ci ;;
       *) echo -e $(colorize RED "Unsupported ROS_REPO=$ROS_REPO. Use 'ros'"); exit 1 ;;
    esac

    echo -e $(colorize BOLD "Starting Docker image: $DOCKER_IMAGE")
-    travis_run docker pull $DOCKER_IMAGE
+
+    if [[ "${ROS_REPO}" == acutronicrobotics ]]; then


this change does not seem to be meaningful

We're using this to force to download the image from our dockerHub.

Yes but travis_run docker pull $DOCKER_IMAGE is run in either case which is equivalent to how things were before this change.

mlautman · 2019-04-08T21:57:35Z

travis.sh

   # For a command that doesn’t produce output for more than 10 minutes, prefix it with travis_run_wait
-   COLCON_CMAKE_ARGS="--cmake-args $CMAKE_ARGS --catkin-cmake-args $CMAKE_ARGS --ament-cmake-args $CMAKE_ARGS"


we still need these no?

mlautman · 2019-04-08T21:58:35Z

travis.sh

   # For a command that doesn’t produce output for more than 10 minutes, prefix it with travis_run_wait
-   COLCON_CMAKE_ARGS="--cmake-args $CMAKE_ARGS --catkin-cmake-args $CMAKE_ARGS --ament-cmake-args $CMAKE_ARGS"
-   travis_run_wait 60 --title "colcon build" colcon build $COLCON_CMAKE_ARGS $COLCON_EVENT_HANDLING
+   travis_run_wait 60 --title "colcon build" colcon build --merge-install $COLCON_CMAKE_ARGS $COLCON_EVENT_HANDLING


do we need to --merge-install or just --symlink-install?

mlautman · 2019-04-08T21:58:53Z

travis.sh

@@ -256,7 +283,7 @@ function test_workspace() {
   blacklist_pkgs=$(filter_out "$source_pkgs" "$all_pkgs")

   # Run tests, suppressing the error output (confuses Travis display?)
-   travis_run_wait --title "colcon test" "colcon test --packages-skip $TEST_BLACKLIST $blacklist $COLCON_EVENT_HANDLING 2>/dev/null"
+   travis_run_wait --title "colcon test" "colcon test --packages-skip $TEST_BLACKLIST $blacklist $COLCON_EVENT_HANDLING --merge-install 2>/dev/null"


same note about --symlink-install vs --merge-install

mlautman · 2019-04-08T21:59:46Z

travis_functions.sh

@@ -31,6 +31,10 @@ travis_nanoseconds() {
    format='+%s000000000'
  fi

+  if [[ "${TRAVIS_OS_NAME}" == osx ]]; then


does this replace the above? I am confused why we have duplicate code now

more comments would be ideal for understanding what this does

mlautman · 2019-04-08T22:02:13Z

util.sh

-  declare -ag _TRAVIS_FOLD_NAME_STACK  # "stack" array to hold name hierarchy
-  declare -Ag _TRAVIS_FOLD_COUNTERS  # associated array to hold global counters
+
+  if [[ "${TRAVIS_OS_NAME}" == osx ]]; then


You should check if the TRAVIS_OS_NAME is not equal to osx

Why should this section be OS-specific at all?
Doesn't osx's bash provide global variables??

rhaschke · 2019-04-09T06:01:56Z

README.md

    cd moveit2

 Next clone the CI script:

-    git clone -b ros2 https://github.com/ros-planning/moveit_ci .moveit_ci
+    git clone -b ros2 https://github.com/acutronicrobotics/moveit_ci .moveit_ci


Same here. Keep the default repo and apply required changes.

rhaschke · 2019-04-09T06:02:17Z

README.md


 Manually define the variables, Travis would otherwise define for you. These are required:

    export TRAVIS_BRANCH=master
    export ROS_DISTRO=crystal
-    export ROS_REPO=ros
+    export ROS_REPO=acutronicrobotics


Do you have your own debian repo?

We're using this variable to download the docker image.

Let's get the necessary fixes into the ros-planning/moveit2 dockerfile and revert this change

rhaschke · 2019-04-09T06:05:03Z

README.md

+
+## Running Locally For Testing in `OS X`
+
+First clone the repo you want to test:


Instead of repeating all instructions here, just highlight the difference to a standard Linux build:
export TRAVIS_OS_NAME=osx

rhaschke · 2019-04-09T06:07:59Z

travis.sh

@@ -27,7 +27,8 @@ COLCON_EVENT_HANDLING="--event-handlers desktop_notification- status-"
 function run_script() {
   local script
   eval "script=\$$1"  # fetch value of variable passed in $1 (double indirection)
-   if [ "${script// }" != "" ]; then  # only run when non-empty
+   if [ ! -z "${script}" ];then
+   #if [ "${script// }" != "" ]; then  # only run when non-empty


Why changing this code? Probably the cleaning of $script was for a reason.

rhaschke · 2019-04-09T06:12:58Z

util.sh

-  declare -ag _TRAVIS_FOLD_NAME_STACK  # "stack" array to hold name hierarchy
-  declare -Ag _TRAVIS_FOLD_COUNTERS  # associated array to hold global counters
+
+  if [[ "${TRAVIS_OS_NAME}" == osx ]]; then


Why should this section be OS-specific at all?
Doesn't osx's bash provide global variables??

davetcoleman · 2019-04-12T18:24:51Z

There's too many changes in this PR. The support for OSX should be in a different PR than the other changes you are making here, for ease of discussion.

Include host network to avoid rosdep issue * ros-industrial/industrial_ci#364 (comment) * https://www.traviscistatus.com/incidents/kyf149kl6bvp?u=3g7kt113nmgs

rhaschke · 2019-05-07T14:01:33Z

As suggested in #56 (comment) and #56 (comment) modification of moveit_ci should be kept to a minimum. But instead of cleaning up this PR, I see even more modifications added to this PR branch. For this reason, I'm going to close this PR and ask Acutronics people to come up with a clean new set of PRs, which we can then hopefully quickly merge.

vmayoral and others added 18 commits April 6, 2019 18:45

Update README so that we use the fork

cd2f3b1

Update rosdep call to allow for fails

9ab02b5

travis.sh: Simplify empty variable check

c8366c8

Merge branch 'ros2' of https://github.com/acutronicrobotics/moveit_ci …

cdcbad7

…into ros2

COLCON_IGNORE failing deps and update README

173949b

travis.sh: Pull image from dockerHub

2e3f39c

Add flags to colcon build

5f35862

This ensures enviromental variables are in place and allows to build moveit_core and other moveit packages

Merge branch 'ros2' of https://github.com/acutronicrobotics/moveit_ci …

d0f4875

…into ros2

Update README.md

815d6d5

Add: export ROS_REPO=acutronicrobotics

Include complete path in COLCON_IGNORE commands

edba617

Merge branch 'ros2' of https://github.com/acutronicrobotics/moveit_ci …

36ebf6e

…into ros2

travis.sh

1884211

Added --merge-install on colcon test command

Merge branch 'ros2' of https://github.com/AcutronicRobotics/moveit_ci …

fc63ace

…into ros2

Merge branch 'ros2' of https://github.com/AcutronicRobotics/moveit_ci …

ce72e16

…into ros2

Add missing dependency for the tests

ee6c3de

Re-enable rosdep, fail everything if dependencies fail

fd6d565

Fetch repos instead

31e903d

This was referenced Apr 8, 2019

Planning request adapter, port to ROS 2 moveit/moveit2#62

Merged

collision_plugin_loader port to ROS 2 moveit/moveit2#69

Closed

CI syncing AcutronicRobotics/moveit2#34

Closed

mlautman suggested changes Apr 8, 2019

View reviewed changes

rhaschke requested changes Apr 9, 2019

View reviewed changes

LanderU and others added 4 commits May 7, 2019 08:59

Update travis.sh

d403158

Include host network to avoid rosdep issue * ros-industrial/industrial_ci#364 (comment) * https://www.traviscistatus.com/incidents/kyf149kl6bvp?u=3g7kt113nmgs

network instead of net

ebcc93b

Update travis.sh

6ce908f

Update travis.sh

7f52de7

rhaschke closed this May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt moveit_ci for moveit2 development #56

Adapt moveit_ci for moveit2 development #56

vmayoral commented Apr 7, 2019 •

edited

Loading

mlautman Apr 8, 2019 •

edited

Loading

LanderU Apr 9, 2019

mlautman Apr 8, 2019

mlautman Apr 8, 2019

mlautman Apr 8, 2019

rhaschke Apr 9, 2019

LanderU Apr 9, 2019

mlautman Apr 8, 2019

LanderU Apr 9, 2019

mlautman Apr 10, 2019

mlautman Apr 8, 2019

mlautman Apr 8, 2019

mlautman Apr 8, 2019

mlautman Apr 8, 2019

davetcoleman Apr 12, 2019

mlautman Apr 8, 2019

rhaschke Apr 9, 2019

rhaschke Apr 9, 2019

rhaschke Apr 9, 2019

LanderU Apr 9, 2019

mlautman Apr 10, 2019

rhaschke Apr 9, 2019

rhaschke Apr 9, 2019

rhaschke Apr 9, 2019

davetcoleman commented Apr 12, 2019

rhaschke commented May 7, 2019

		# For a command that doesn’t produce output for more than 10 minutes, prefix it with travis_run_wait
		COLCON_CMAKE_ARGS="--cmake-args $CMAKE_ARGS --catkin-cmake-args $CMAKE_ARGS --ament-cmake-args $CMAKE_ARGS"


		## Running Locally For Testing in `OS X`

		First clone the repo you want to test:

Adapt moveit_ci for moveit2 development #56

Adapt moveit_ci for moveit2 development #56

Conversation

vmayoral commented Apr 7, 2019 • edited Loading

mlautman Apr 8, 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

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

davetcoleman commented Apr 12, 2019

rhaschke commented May 7, 2019

vmayoral commented Apr 7, 2019 •

edited

Loading

mlautman Apr 8, 2019 •

edited

Loading