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

Moveit2 ci #46

Merged
merged 3 commits into from
Mar 25, 2019
Merged

Moveit2 ci #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8fea56
update rosinstall
mlautman Mar 15, 2019
7e0fc1c
porting dockerfile in preparation for moveit2-ci
Mar 12, 2019
0ab2a54
changing branch from moveit2-ci to master in prep for merge
Mar 25, 2019
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
15 changes: 0 additions & 15 deletions .docker/ci-shadow-fixed/Dockerfile
View file
Open in desktop

This file was deleted.

97 changes: 68 additions & 29 deletions .docker/ci/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,89 @@
# moveit/moveit:melodic-ci
# moveit/moveit2:crystal-ci
# Sets up a base image to use for running Continuous Integration on Travis
FROM ros:crystal-ros-base-bionic

FROM ros:melodic-ros-base
MAINTAINER Dave Coleman dave@picknik.ai
LABEL maintainer="mike@picknik.ai"

ENV TERM xterm

# Setup catkin workspace
ENV CATKIN_WS=/root/ws_moveit
WORKDIR $CATKIN_WS
ENV ROS_WS=/opt/ws_moveit
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 sure why you changed the original location at all. But /opt seems to be rather weird for a temporary and private folder. I suggest to use ros_ws as folder name (as everywhere else now) and /tmp to indicate temporary usage.

Suggested change
ENV ROS_WS=/opt/ws_moveit
ENV ROS_WS=/tmp/ros_ws

Copy link
Contributor

Choose a reason for hiding this comment

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

Developers often mount /tmp in the container to an external volume to cache filesystems, proxies, and workspaces between the host. I don't think it would helpful to place the workspace that the Dockerfile is to build at the only root folder that is frequently overloaded from docker volumes.


# 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 && \
WORKDIR $ROS_WS

# Update apt package list as previous containers clear the cache
RUN apt-get -qq update && \
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 these RUN commands should be combined again eventually.

Copy link
Member

Choose a reason for hiding this comment

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

But that would negatively impact layer reuse/caching (see above).

apt-get -qq dist-upgrade -y && \
#
# Install some base dependencies
apt-get -qq install -y \
# Some source builds require a package.xml be downloaded via wget from an external location
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!
ccache \
curl gnupg2 lsb-release \
# Some python dependencies for working with ROS2
python3-colcon-common-extensions \
python3-pip \
python-rosdep \
python3-wstool \
python3-rospkg-modules \
mlautman marked this conversation as resolved.
Show resolved Hide resolved
python3-rosdistro-modules \
&& \
# Clear apt-cache to reduce image size
rm -rf /var/lib/apt/lists/*
mlautman marked this conversation as resolved.
Show resolved Hide resolved

# Upgrade Pip and install some packages needed for testing
# NOTE(mlautman): These pip installs are from the ros2 source install instructions. Seeing as not all of them
# are installed in the base image I added them here. I have not been able to verify that they are needed
# and if they aren't necessary they should be removed.
RUN python3 -m pip install --upgrade pip && \
python3 -m pip install -U \
mlautman marked this conversation as resolved.
Show resolved Hide resolved
argcomplete \
flake8 \
mlautman marked this conversation as resolved.
Show resolved Hide resolved
flake8-blind-except \
flake8-builtins \
flake8-class-newline \
flake8-comprehensions \
flake8-deprecated \
flake8-docstrings \
flake8-import-order \
flake8-quotes \
pytest-repeat \
pytest-rerunfailures \
pytest \
pytest-cov \
pytest-runner \
setuptools

# Install Fast-RTPS dependencies
RUN apt-get -qq update && \
rhaschke marked this conversation as resolved.
Show resolved Hide resolved
apt-get install -qq --no-install-recommends -y \
libasio-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 could be moved to the 'apt-get install...' block above so we don't need to call an extra update

Copy link
Member

Choose a reason for hiding this comment

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

Splitting apt-get install .. commands like this is accepted (and best) practice in Dockerfiles. It allows for better reuse of cached layers.

libtinyxml2-dev \
&& \
# Clear apt-cache to reduce image size
rm -rf /var/lib/apt/lists/*

# Download moveit source, so that we can get necessary dependencies
RUN mkdir src && \
cd $ROS_WS/src && \
mlautman marked this conversation as resolved.
Show resolved Hide resolved
# TODO(mlautman): This should be changed to use vcs in the future
wstool init --shallow . https://raw.githubusercontent.com/ros-planning/moveit2/master/moveit.rosinstall

# Download all MoveIt dependencies
RUN \
rhaschke marked this conversation as resolved.
Show resolved Hide resolved
apt-get -qq update && \
Copy link
Member

Choose a reason for hiding this comment

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

we just ran update, no?

Copy link
Member

@gavanderhoorn gavanderhoorn Mar 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but rm -rf /var/lib/apt/lists/* removed the local apt indices (again best practice to avoid layer bloat).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Having separate apt-get commands and removing the apt caches, avoids layer bloat.
However, it leaves us with multiple update commands, downloading the caches.
I'm fine with it, if this is best-practice nowadays. Probably dockerhub has a good network connection 😉

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/*

# Remove the source code from this container
# TODO: in the future we may want to keep this here for further optimization of later containers
RUN cd $ROS_WS && \
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole ROS_WS can be removed, right?

This command should be part of the same command downloading the MoveIt sources.
As @davetcoleman teached me a year? ago, commands creating and removing files/folders should be kept together in a single docker RUN command. For each RUN command a new image layer is added for all filesystem changes in this command. So, if we add and remove files in separate commands, we need to save and download the layer caches for both, additions and removals, while if both shell commands are handled in the same docker command, there are virtually no changes to save and download.

Copy link
Contributor

Choose a reason for hiding this comment

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

In view of #46 (comment), I think keeping separate commands for apt-get is fine.
However, downloading MoveIt sources, installing dependencies via rosdep, and cleaning should all be handled in a single command then to minimize layer size.
Do you agree @ruffsl / @gavanderhoorn?

Copy link
Contributor

Choose a reason for hiding this comment

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

However, downloading MoveIt sources, installing dependencies via rosdep, and cleaning should all be handled in a single command then to minimize layer size.

Given that we are now cleaning up after ourselves after each RUN invocation and apt installation, there is little tangible difference in overall size, while at the same time we gain finer granularity in our build caching.

The whole ROS_WS can be removed, right?

Acutely, I think we should omit this sanitization step entirely. It's rather valuable to have the exact source code on hand in the image that was used in building the binaries in the workspace. In navigation2, we use the existing source in the workspace baked into the image to seed the keys for later CI caching, as it worthwhile be able to determine if source code dependencies have changed from the latest pull. Cleaning the existing source I think is best left to the CI script that is to use the image, as we do via a pre_checkout step here:

https://github.com/ros-planning/navigation2/blob/bbbaf6d0cde05e89db871601677f134a823e13b3/.circleci/config.yml#L3-L11

Copy link
Contributor

@rhaschke rhaschke Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For this use case, we have a different docker container: source.
Here, the workspace is only needed to pull in dependencies! We don't (yet) compile any binaries in this docker image.

rm -rf src/ && \

# Continous Integration Setting
ENV IN_DOCKER 1
28 changes: 0 additions & 28 deletions .docker/experimental/Dockerfile
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions .docker/release/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# moveit/moveit:melodic-release
# moveit/moveit2:crystal-release
# Full debian-based install of MoveIt! using apt-get
# TODO(mlautman): Add this Dockerfile to DockerHub once the moveit2 debians are released

FROM ros:melodic-ros-base
FROM ros:crystal-ros-base-bionic
mlautman marked this conversation as resolved.
Show resolved Hide resolved
MAINTAINER Dave Coleman dave@picknik.ai

# Commands are combined in single RUN statement with "apt/lists" folder removal to reduce image size
Expand Down
28 changes: 11 additions & 17 deletions .docker/source/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,24 @@
# moveit/moveit:melodic-source
# moveit/moveit2:crystal-source
# Downloads the moveit source code and install remaining debian dependencies

FROM moveit/moveit:melodic-ci-shadow-fixed
MAINTAINER Dave Coleman dave@picknik.ai
FROM moveit/moveit2:crystal-ci
LABEL maintainer="mike@picknik.ai"

# 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 && \
RUN mkdir src && \
cd src && \
#
# Download moveit source so that we can get necessary dependencies
wstool init . https://raw.githubusercontent.com/ros-planning/moveit/${ROS_DISTRO}-devel/moveit.rosinstall && \
#
# Update apt package list as cache is cleared in previous container
# Usually upgrading involves a few packages only (if container builds became out-of-sync)
apt-get -qq update && \
wstool init . https://raw.githubusercontent.com/ros-planning/moveit2/master/moveit.rosinstall

# Update apt package list as cache is cleared in previous container
# Usually upgrading involves a few packages only (if container builds became out-of-sync)
RUN apt-get -qq update && \
apt-get -qq dist-upgrade && \
#
rosdep update && \
rosdep install -y --from-paths . --ignore-src --rosdistro ${ROS_DISTRO} --as-root=apt:false && \
rm -rf /var/lib/apt/lists/*

# Configure catkin workspace, but do not build because Docker times out
ENV PYTHONIOENCODING UTF-8
RUN catkin config --extend /opt/ros/$ROS_DISTRO --install --cmake-args -DCMAKE_BUILD_TYPE=Release
# Status rate is limited so that just enough info is shown to keep Docker from timing out,
# but not too much such that the Docker log gets too long (another form of timeout)
# catkin build --limit-status-rate 0.001 --no-notify
# Build the workspace
RUN colcon build --symlink-install --event-handlers console_direct+
63 changes: 42 additions & 21 deletions moveit.rosinstall
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,55 @@
# This file is intended for users who want to build MoveIt! from source.
# Used with wstool, users can download source of all packages of MoveIt!.

- git:
local-name: moveit
uri: https://github.com/ros-planning/moveit.git
local-name: moveit2
uri: https://github.com/ros-planning/moveit2.git
version: master
- git:
local-name: moveit_msgs
uri: https://github.com/ros-planning/moveit_msgs.git
version: melodic-devel
# TODO(mlautman): update to ros-planning once the package has been merged back in
uri: https://github.com/AcutronicRobotics/moveit_msgs
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: ros2
- git:
local-name: moveit_resources
uri: https://github.com/ros-planning/moveit_resources.git
version: master
local-name: geometric_shapes
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/geometric_shapes
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: ros2
- git:
local-name: moveit_visual_tools
uri: https://github.com/ros-planning/moveit_visual_tools.git
version: melodic-devel
local-name: object_recognition_msgs
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/object_recognition_msgs
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: master
- git:
local-name: rviz_visual_tools
uri: https://github.com/PickNikRobotics/rviz_visual_tools
version: melodic-devel
local-name: octomap_msgs
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/octomap_msgs
version: ros2
- git:
local-name: geometric_shapes
uri: https://github.com/ros-planning/geometric_shapes.git
version: melodic-devel
local-name: random_numbers
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/random_numbers
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: ros2
- git:
local-name: srdfdom
uri: https://github.com/ros-planning/srdfdom.git
version: melodic-devel
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/mlautman/srdfdom
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: ros2
- git:
local-name: urdf_parser_py
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/urdf_parser_py
version: ros2
- git:
local-name: octomap
uri: https://github.com/AcutronicRobotics/octomap
version: ros2
- git:
local-name: moveit_resources
# TODO(mlautman): update to ros-planning once the package has been ported
uri: https://github.com/AcutronicRobotics/moveit_resources
mlautman marked this conversation as resolved.
Show resolved Hide resolved
version: master
- git:
local-name: moveit_tutorials
uri: https://github.com/ros-planning/moveit_tutorials.git
version: melodic-devel
local-name: joint_state_publisher
uri: https://github.com/ros/joint_state_publisher.git
version: ros2-devel