Update Docker build and add CircleCI + CodeCov support #44
Conversation
leveraging nightly docker image
|
@ruffsl thanks for starting this! I need to do further analysis, but the ycombinator article you shared was very interesting. At today's sync up meeting between Acutronic and PickNik we were debatting if we should continue to use Travis-based moveit_ci. Do we need both? Our moveit_ci provides a lot of cool stuff:
Can CircleCI do all of this? I'd be happy to have a follow-up call with you, also. |
| @@ -0,0 +1,82 @@ | |||
| # This dockerfile expects proxies to be set via --build-arg if needed | |||
There was a problem hiding this comment.
we have a .docker folder:
https://github.com/ros-planning/moveit2/tree/master/.docker
There was a problem hiding this comment.
Optimizations of the CI config heavily rely upon the assumed setup of this Dockerfile, so they go hand-in-hand. Perhaps @mlautman could mention what changes should be added into this CI dockerfile?
There was a problem hiding this comment.
I would recommend testing with the .docker/ci/Dockerfile in the MoveIt2 Ci PR as it is very close to being merged in.
There was a problem hiding this comment.
I made some comments on #46 (review) that the Dockerfile here also addresses. However, like I mentioned, the Dockerfile here and config go rather hand-in-hand due to the build caching strategy optimizations. That said, I'm not opposed to multiple dockerfiles; we could have one for the existing travis setup and one for CircleCI.
There was a problem hiding this comment.
We certainly can have multiple (we currently have about 4) but it is important that we remove redundancies wherever possible. Looking through the Dockerfile (albeit rather briefly) it seems that the docker container created isn't very different from moveit/moveit2:crystal-source The main difference that I see is that this Dockerfile builds the workspace whiel crystal-source does not. (Although it certainly could)
There was a problem hiding this comment.
@davetcoleman and myself were just discussing this and it makes sense, at least in the short term, to have multiple dockerfiles with one for the existing travis setup and one for CricleCi
There was a problem hiding this comment.
Agreed, I would also like to prune Dockerfile redundancies. I have an idea to address this later using build args in the FROM calls and overriding the Dockerhub build hooks. See bfdbc03 and https://docs.docker.com/docker-hub/builds/advanced as I linked to above with https://github.com/ruffsl/navigation2/tree/foo/.dockerhub/devel
There was a problem hiding this comment.
Can we move this file to our .docker folder at least?
There was a problem hiding this comment.
| # This dockerfile expects proxies to be set via --build-arg if needed | |
| # Docker specifically for CircleCi setup, not for Travis | |
| # | |
| # This dockerfile expects proxies to be set via --build-arg if needed |
There was a problem hiding this comment.
Can we move this file to our .docker folder at least?
The Dockerfile is kept in the root of the project so that the build context may encompass the entire repo so that we can COPY all the separate source files into the image. It is possible to have different Dockerfile and build context locations, but from a user perspective, the expected build context location is no longer obvious. DockerHub has recently revamped the build rules setting, allowing us to specify separate paths, and we can document the use required pwd and --file for uses building locally.
https://docs.docker.com/engine/reference/commandline/build/#extended-description
https://docs.docker.com/engine/reference/commandline/build/#text-files
tools/code_coverage_report.bash
Outdated
| @@ -0,0 +1,53 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
im so excited to have code coverage reports!
There was a problem hiding this comment.
This is borrowed over from navigation2 and ros-navigation/navigation2#553 .
Perhaps @crdelsey would have some recommendations for your cmake setup?
There was a problem hiding this comment.
To have a single COVERAGE_ENABLED flag passed as a CMake argument, as @ruffsl has done in this PR, you need to add a few lines to every package's top level CMakelist.txt
option(COVERAGE_ENABLED "Enable code coverage" FALSE)
if(COVERAGE_ENABLED)
add_compile_options(--coverage)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} --coverage")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} --coverage")
endif()Rather than duplicate this code everywhere, we created a package to hold common CMake macros.
https://github.com/ros-planning/navigation2/tree/master/nav2_common
This package defines a macro with our standard package header, and we just call that in each package. So instead of adding all the lines above, we add two lines to each package:
find_package(nav2_common) # for the macro definintionand then after all the find_package lines
nav2_package() # to bring in standard package configuration options including COVERAGE_ENABLEDThere was a problem hiding this comment.
@davetcoleman would you like to me to port something like movit2_common into this PR?
There was a problem hiding this comment.
That seems like a great optimization. Does it make sense to make a moveit2_common package or would it be better to rename the nav2_common package to something more general like ament_code_coverage and include that?
Co-Authored-By: ruffsl <roxfoxpox@gmail.com>
|
Thank you again for helping migrate MoveIt from travis to CircleCI! |
.docker/ci/Dockerfile
Outdated
| # Example build command: | ||
| # export http_proxy=http://my.proxy.com:80 | ||
| # export CMAKE_BUILD_TYPE=Debug | ||
| # docker build -t nav2:latest --build-arg http_proxy --build-arg CMAKE_BUILD_TYPE ./ |
There was a problem hiding this comment.
I omitted the org so user building don't blow away what they may have pulled.
There was a problem hiding this comment.
Makes sense. maybe change nav2->moveit2?
Also I really like that the existing dockerfiles have their label in the documentation (eg https://github.com/ros-planning/moveit2/blob/master/.docker/ci/Dockerfile#L1) do you think you can add a simmilar line up top? It could be something like moveit/moveit2:crystal-circle-ci
There was a problem hiding this comment.
Rather than have some manual mapping from github branches to dockerhub tags, like crystal-circle-ci, I'd recommend we adopt a more programmatic style where release branches and tags on ros-planning/moveit2 github are mapped directly to tag names on ros-planning/moveit2 for CI consumption. There can be procedurally created, while the origin/relation of docker tags to code would be clear and more amenable for CI configs; alluded to in #44 (comment)
.docker/ci/Dockerfile
Outdated
| @@ -1,50 +1,81 @@ | |||
| # moveit/moveit:melodic-ci | |||
There was a problem hiding this comment.
Let's keep this file intact as it is already setup with dockerhub and travis. Perhaps we could move your changes to .docker/circle-ci/Dockerfile. Once that is done I am comfortable giving this a +1.
On a related note, if you have access to the ros-planning repo, I would recommend pushing your branch to ros-planning/moveit2 so that we can test that everything works with Dockerhub. I did the same thing for my PR #46 and it has been really helpful for iterating quickly.
There was a problem hiding this comment.
I've restored the old Dockerfile in 6d642ca , but reminded I had to have new Dockerfile at the root of the project so that the build context may encompass the entire repo that it copies from. I've added a .dockerignore file to mitigate build cache breaks from git fetches or editing of the Dockerfile itself.
So that local copy operation my succeed Given the build context must include the repo root
to avoid unnecessarily breaking the build cache
This reverts commit da3173c.
Co-Authored-By: Dave Coleman <dave@picknik.ai>
|
@mlautman this seems mostly done to me, except for the repos file maybe. What's missing to get this merged? |
henningkayser
moved this from In progress
to Review in progress
in MoveIt2 Beta - Phase 1
|
As this repo hasn't been active for 3 months, the CircleCI config we use over on ros-planning/navigation2 that this PR derives from has advanced quite a bit. In the coming days I'll try and refactor those fixes to this PR. However I'm guessing the build state of the repo, that I could used to test the CI with, hasn't changed much since June. |
@ruffsl, thanks a lot for looking into this again! |
There was a problem hiding this comment.
I shortly skimmed through this. Some lcov commands are interesting to port over into moveit_ci.
Otherwise, I think we can (and should) keep our hierarchy of MoveIt1 docker files that are optimized for fast CI building. Particularly, the proposed approach here requires to rosdep install all packages for each CI build from scratch, doesn't it?
CircleCI's configuration seems to be amazingly complex. I'm not sure, we should switch from Travis at all.
The advantages of this approach as far as I can tell is actually slight better (and worse) caching. With this approach the CI run is done in stages and a hash of the input of each stage is calculated. If possible (all those inputs existed before) it uses a cache of the output of that sage. Specifically if the external ros dependencies installed via debians or built from source haven't changed they can be re-used (right now we sort of have that with our ci docker images except we have to rebuild and push them every so often, unless that's done automatically when the ros dependencies update). Next, if that first stage (ros dependencies) was able to be reused from a cache it does the same thing with the upstream workspace. If there was no changes in the inputs for the upstream workspace it can just re-use it from a cache. Lastly for the target workspace it can do the same thing. This would happen if you re-ran a job with no code changes it would basically just skip to the testing step and run the tests. To get that type of caching we would need a ci system that supported pipeline-ing in the way that circle-ci is built to support. Since this was written CircleCI has the idea of Orbs which are CI routines (similar to what we essentially have in moveit_ci / industrial_ci). These CI orbs can be published and re-used between projects that just specify parameters to the Orb. Reference: https://circleci.com/orbs/ This is the same sort of workflow that Gitlab CI is now supporting. Because of this (and my desire to see faster ci runs) I've submitted an issue to start the discussion of supporting pipeline style ci runs (and therefore caching of steps in the ci process) I've opened this issue: ros-industrial/industrial_ci#548 Above I said worse caching. What I mean by that is this routine here doesn't support caching ccache outputs. The reason I think this is has to do with the fact that caches in circle_ci are immutable. I think this has to do with how CircleCI internally prunes caches. This seems specifically opposed to the sort of caching that we do with ccache. It should be possible to get the ccache caching outside of circleci's caching system if we used s3 for it. I made a small attempt at using and learning about CircleCI yesterday and this is what I learned. I would love to be proved wrong on the ccache issue. @ruffsl I'm curious how the navigation2 CI system has changed and if this could be abstracted into a CircleCI Orb we could try to re-use and standardize on. @rhaschke there is some interesting work in industrial_ci right now to support a more feature full approach to code coverage reporting. It would be really nice if we could find someway to abstract some of this functionality so we can all benefit from each-other's work. I'm not sure what the best way to do that is, but one way would be to integrate with industrial_ci and add the features to support a pipeline workflow. Here is a link to the PR over there for code coverage reporting: ros-industrial/industrial_ci#504 |
|
One big advantage I see from supporting this sort of caching is if you had two developers submit patches around the same time (where none of the upstream dependencies would have changed) you would basically get to skip that step. There is a chance (unknown to me) that travis supports this sort of caching and pipe-lining too, we could also roll it ourselves (yuck). |
|
The answer to the ccache problem was right in front of me in navigation2 (now, not in this pr). I wonder if we can abstract the circleci config.yaml into an Orb so we can standardize how to do this. |
|
@ruffsl reading through the CircleCI documentation I don't understand some of your syntax. My questions relate to this file: https://github.com/ros-planning/navigation2/blob/master/.circleci/config.yml For starters I can't find the keyword |
|
I take back what I said above... I don't think you are using ccache caching as optimally as you could be. Also, because of the symbolic link you put in the underlay workspace to /opt/ros/$ROS_DISTRO you don't build the underlay and always skip it in setup. I'm a bit confused by that. As to ccache you are tagging the cache for it with the run which means that every build you do in circle ci is with an empty cache. Only if in the same run (due to bad copying of a workspace or something) you re-build something would ccache be helping you. You should instead specify multiple keys according to the documentation of circleci to enable picking the most recent cache with the base tag. (This should greatly speed up builds done on master after a merge of a PR). |
|
Another thing I think I found is that you are using the caching system instead of persist_to_workspace where that would be a better fitting tool for what you want (when you re-use a workspace from the debug on the release tests). |
As I've just commented in a ticket elsewhere, with respect to the current state of the Nav2 CI, I'd recommend reading through the docs I've been adding on the subject. It should correct you idea on some of the concepts and features we've been leveraging with CircleCI. Perhaps you may want to comment on the PR with more specific questions after you've read through it, as there may still be some technical details that to me as the author seem self evident, but not so for someone just approaching this.
If there's interest, I can revisit this and quickly back port the improvements I've made to the Nav2 CI to this PR (or just start a new one by now, haha). I'm not sure how much we could push into an orb. Orbs are kind of clunky, given how they are imported, and scope. A lot of the optimizations also come from the style of Docker base image, which an orb wouldn't subplant. As in the other ticket, I'd like to work towards GH actions, given the worker client software is OSS. But last I checked GH actions, the dev story for caching and test results was a bit too under cooked 6 months ago, but that may have changed. I was waiting for a few of the more polished asure CI feature from Microsoft to roll over in the GH actions. |
|
As to GH Actions. industrial_ci already supports it natively and it works fairly well. My motivation to figure out the CircleCI system is that moveit has really long ci times and I think that with a careful use of the caching we could greatly reduce the ci run times. |
|
@tylerjw now that migration to Github Actions is basically done, should we close this PR? It is our oldest outstanding PR and does not seem to have momentum anymore. |
I'll close it. Please re-open this PR if you'd like to resume working on it or using work from it to form a new PR. |
* github -> GitHub * Rviz -> RViz, and rviz -> RViz * moveit -> MoveIt! * spelling fixes across repo * cartesian -> Cartesian * misc spelling fixes
This PR begins the process updating legacy docker build process and adding support for CircleCI, addressing #24 . This falls inline as with other ros2 projects adopting alternative CI integrations as Travis inevitably winds down due to it acquisition from Idera: https://news.ycombinator.com/item?id=19218036
Additional motivation and features are expounded upon in navigation2's respective PR:
ros-navigation/navigation2#534
Action Items
DockerHub
ros-planningDockerHub organization (admin)moveit2automated repo to DockerHub org (admin)ros-planning/moveit2:masterosrf/ros2:nightlyCircleCI
CodeCov
Ping @davetcoleman , @mlautman , @vmayoral
As of writing, none of the ROS2 packages within this moveit2 repo build yet, so we have nothing to test.