Moveit2 ci#46
Conversation
There was a problem hiding this comment.
A few minor remarks.
Furthermore, I suggest to factor out the COLCON_IGNORE files into a separate PR.
They are not related to the Dockerfiles, are they?
If we merge-commit this PR, it should be fine though as you have split the individual commits.
|
@rhaschke @davetcoleman Changes have been applied and dockerhub is now successfully building moveit/moveit:crystal-ci Ready for merge? (after changing the url's to master branch) |
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
A few remaining suggestions...
|
@rhaschke When you are happy with this PR, feel free to update the url's and squash-merge. Or I can take care of it |
henningkayser
left a comment
henningkayser
left a comment
There was a problem hiding this comment.
Just some minor points before I approve
.docker/ci/Dockerfile
Outdated
| flake8-docstrings \ | ||
| flake8-import-order \ | ||
| flake8-quotes \ | ||
| # git+https://github.com/lark-parser/lark.git@0.7d \ |
| # Install Fast-RTPS dependencies | ||
| RUN apt-get -qq update && \ | ||
| apt-get install -qq --no-install-recommends -y \ | ||
| libasio-dev \ |
There was a problem hiding this comment.
These could be moved to the 'apt-get install...' block above so we don't need to call an extra update
There was a problem hiding this comment.
Splitting apt-get install .. commands like this is accepted (and best) practice in Dockerfiles. It allows for better reuse of cached layers.
|
|
||
| # Download all MoveIt dependencies | ||
| RUN \ | ||
| apt-get -qq update && \ |
There was a problem hiding this comment.
Yes, but rm -rf /var/lib/apt/lists/* removed the local apt indices (again best practice to avoid layer bloat).
There was a problem hiding this comment.
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 😉
| WORKDIR $ROS_WS | ||
|
|
||
| # Update apt package list as previous containers clear the cache | ||
| RUN apt-get -qq update && \ |
There was a problem hiding this comment.
I think these RUN commands should be combined again eventually.
There was a problem hiding this comment.
But that would negatively impact layer reuse/caching (see above).
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
I really would like to merge these asap, but you introduced some important performance regressions. Please talk to Dave about these issues first. If he agrees to merge this PR anyway to progress on CI, that's fine for me. But you should tackle those issues immediately after in another PR.
| # Setup catkin workspace | ||
| ENV CATKIN_WS=/root/ws_moveit | ||
| WORKDIR $CATKIN_WS | ||
| ENV ROS_WS=/opt/ws_moveit |
There was a problem hiding this comment.
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.
| ENV ROS_WS=/opt/ws_moveit | |
| ENV ROS_WS=/tmp/ros_ws |
There was a problem hiding this comment.
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.
|
|
||
| # 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 && \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
|
Linking to #46 (comment) as it was prematurely resolved. Do we really want to lose the traceability in determining the source code that was used in crating the workspace binaries? |
henningkayser
left a comment
henningkayser
left a comment
There was a problem hiding this comment.
All right, I think this PR is good for now and can be merged.
|
😛 |
mlautman
dismissed
rhaschke’s stale review
Let's tackle the remaining open questions in a separate PR
@mlautman, I hope you will do so. I'm tired to clean up after your PRs. |
Port plan_execution
7 participants
Description
TODO: resolve with #44 and #37
Create a moveit_ci branch that passes travis.
Ports the MoveIt ci Dockerfile to ROS2
Removes shadow-fixed since ROS2 does not provide shadow-fixed packages
Removes experimental docker recipe since it is not available for ROS2
Checklist