-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[MERGE AFTER #1982] Integrate ament into CI and docker builds #2007
[MERGE AFTER #1982] Integrate ament into CI and docker builds #2007
Conversation
Unify docker support with a new (and more generic) set of docker files. Two main images are created: - Base image containing an Ubuntu installation with all dependencies - Build image containing a pre-built Autoware When Cuda support is enabled, there is a further Base+Cuda support image. These Dockerfiles can be used for both AArch64 and x86_64 images. Co-authored-by: Filipe Rinaldi <filipe.rinaldi@arm.com>
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.
HI Maxim, here are a few comments on the patch...
@@ -69,8 +69,12 @@ RUN groupadd --gid $GROUP_ID $USERNAME && \ | |||
chmod 0440 /etc/sudoers.d/$USERNAME | |||
|
|||
# Startup scripts | |||
ARG ROS_DISTRO |
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.
ROS_DISTRO is already defined at the top of the file before the FROM statement.
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 think you need to repeat ARG if using it FROM as the first one had a scope of the following FROM command only - https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact
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.
Just a minor thing, the packages for apt-get install should be in alphabetical order. Otherwise, the patch is a +1 from me on the Dockerfile changes.
docker/generic/Dockerfile.base
Outdated
# | ||
# Enable ament_cmake | ||
# | ||
|
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.
We use FROM ros:$ROS_DISTRO here, so ROS GPG key is already imported:
apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys 421C365BD9FF1F717815A3895523BAEEB01FA116 Executing: /tmp/tmp.3Qb23No1Wz/gpg.1.sh --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys 421C365BD9FF1F717815A3895523BAEEB01FA116 gpg: requesting key B01FA116 from hkp server keyserver.ubuntu.com gpg: key B01FA116: "ROS Builder " not changed gpg: Total number processed: 1 gpg: unchanged: 1
docker/generic/Dockerfile.base
Outdated
# Enable ament_cmake | ||
# | ||
|
||
RUN echo "deb http://repo.ros2.org/ubuntu/main xenial main" | tee /etc/apt/sources.list.d/ros2-latest.list |
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.
Replace xenial with lsb_release -sc
to simplify next time we update the base OS. You may need to add "lsb-release" to the installation list on line 10.
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.
Yep
docker/generic/Dockerfile.base
Outdated
# | ||
|
||
RUN echo "deb http://repo.ros2.org/ubuntu/main xenial main" | tee /etc/apt/sources.list.d/ros2-latest.list | ||
RUN apt-get update |
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.
Combine apt-get update and apt-get install on the same RUN command to avoid bloating the Docker layers. Also add "rm -rf /var/lib/apt/lists/*" at the end of the same RUN command.
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.
Yep
docker/generic/build.sh
Outdated
@@ -78,6 +79,7 @@ BASE=$IMAGE_NAME:$TAG_PREFIX-$ROS_DISTRO-base | |||
docker build \ | |||
--tag $BASE \ | |||
--build-arg ROS_DISTRO=$ROS_DISTRO \ | |||
--build-arg ROS2_DISTRO=${ROS2_DISTRO} \ |
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.
You may drop the curly brackets syntax to be consistent with the rest of the file.
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.
Yep
docker/generic/hooks/build
Outdated
@@ -9,6 +9,7 @@ echo "[***] Build hook running" | |||
docker build \ | |||
-t $BASE \ | |||
--build-arg ROS_DISTRO=$ROS_DISTRO \ | |||
--build-arg ROS2_DISTRO=${ROS2_DISTRO} \ |
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.
Drop curly brackets.
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.
Yep
d373157
to
f8b33b2
Compare
@filiperinaldi Comments addressed, thank you! |
Signed-off-by: Maxim Osipov <maxim.osipov@gmail.com>
f8b33b2
to
d8937b8
Compare
Gitlab CI pipeline: |
Status
PRODUCTION / DEVELOPMENT
Description
In order to move packages to ament_cmake in preparation for ROS2 transition we need to enable support for ament in the build system.
Related PRs
#1982 autowarefoundation/autoware_ai#530 #1946 autowarefoundation/autoware_ai#551
Todos