Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Update Docker build and add CircleCI + CodeCov support #44
Update Docker build and add CircleCI + CodeCov support #44
Changes from all commits
0b350f9
826b152
1118615
b6f4a93
ac10b22
95c3036
da182b3
4073586
e93c8b9
c5d41c1
da3173c
7d2b7ac
fc71c65
8a5f875
f42ba65
bfdbc03
4cff4af
bf95820
adc87a7
6d642ca
5940894
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when? or remove comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is code coverage working now? I was waiting on this until this repo uses something like nav2's cmake changes to enable codecov at build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this improves caching, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I the
.md
and.txt
files are inconsequential to the build, perhaps is worth blanketing them all in the root directory?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a .docker folder:
https://github.com/ros-planning/moveit2/tree/master/.docker
@mlautman has been setting up docker for ROS2:
#46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this file to our
.docker
folder at least?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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