Refactor Devcontainer Dockerfile (Root User/Bind Mounts) and Restores GitHub Actions Workflows#19
Refactor Devcontainer Dockerfile (Root User/Bind Mounts) and Restores GitHub Actions Workflows#19
Conversation
cooperj
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to fix the devcontainer build by running privileged install steps as root, avoid copying the entire workspace into the devcontainer image by using BuildKit bind mounts, and re-enable/adjust GitHub Actions workflows for devcontainer and Docker image builds.
Changes:
- Refactors
.devcontainer/Dockerfileto run apt installs asroot, switch users, and use a bind mount forrosdep installinstead of copying the whole repo. - Re-enables/adds GitHub Actions workflows for ROS CI and Docker image builds, and simplifies the devcontainer CI build step by removing cache configuration.
- Adds a GHCR build/push workflow for the “final” Docker image target.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.devcontainer/Dockerfile |
Switches to root for apt, uses bind mount for rosdep, and keeps devcontainer/final stages. |
.github/workflows/dev-container.yml |
Removes build cache configuration from the devcontainer CI action invocation. |
.github/workflows/ros-ci.yml |
Adds a ROS CI workflow running builds in container images for multiple ROS distros. |
.github/workflows/build-docker.yml |
Adds a workflow to build/push the “final” Docker image to GHCR with metadata and caching. |
Comments suppressed due to low confidence (1)
.devcontainer/Dockerfile:34
- The
COPY ./ /workspace/was removed, but the image still runscolcon buildin this stage and again in thefinalstage. Without copying (or bind-mounting) the workspace for those build steps,/workspace/srcand/workspace/installwon’t exist in the built image, and theCMDthat sources/workspace/install/setup.bashwill fail. Recommended: keep the bind-mount approach forrosdepin the devcontainer stage, but add aCOPY(or at leastCOPY src/) in thefinalstage beforecolcon build, and either remove the initial build indevcontaineror also run it with a bind mount.
WORKDIR /workspace
ENV ROS2_WS=/workspace
RUN --mount=type=bind,source=.,target=/workspace \
rosdep update --rosdistro ${ROS_DISTRO} &&\
apt-get update && \
rosdep install --from-paths /workspace/src --ignore-src -r -y
# Do an initial build to speed up development
RUN /bin/bash -lc "source /opt/ros/${ROS_DISTRO}/setup.bash && colcon build --continue-on-error"
# ----------------------------------------------------------------------------------------------------
FROM devcontainer AS final
RUN /bin/bash -lc "source /opt/ros/${ROS_DISTRO}/setup.bash && colcon build"
CMD ["/bin/bash", "-lc", "source /workspace/install/setup.bash && exec ros2 launch example example.launch.py"]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ENV main_user=${USER} | ||
|
|
||
| USER root | ||
| RUN rm -rf /var/lib/apt/lists/* \ | ||
| && apt-get update --fix-missing \ | ||
| && apt-get -y install --no-install-recommends --fix-missing \ | ||
| ros-${ROS_DISTRO}-desktop \ | ||
| bash-completion \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY ./ /workspace/ | ||
| WORKDIR /workspace | ||
| USER ${main_user} |
There was a problem hiding this comment.
ENV main_user=${USER} is likely to expand to an empty value during build (the base image may not define a USER environment variable). That can make USER ${main_user} invalid and break the build. Consider explicitly setting the intended username via an ARG (with a sane default like root) or hard-coding the user you want to switch back to, rather than relying on $USER.
There was a problem hiding this comment.
@cooperj copilot is right... we need to ensure that main_user is set to a value, so I suggest
ENV main_user=${USER:-root}so that if no $USER is defined it is set to root
| USER ${main_user} | ||
|
|
||
| WORKDIR /workspace | ||
| ENV ROS2_WS=/workspace | ||
| RUN rosdep update --rosdistro ${ROS_DISTRO} &&\ | ||
| RUN --mount=type=bind,source=.,target=/workspace \ | ||
| rosdep update --rosdistro ${ROS_DISTRO} &&\ | ||
| apt-get update && \ | ||
| rosdep install --from-paths ./src --ignore-src -r -y | ||
| rosdep install --from-paths /workspace/src --ignore-src -r -y |
There was a problem hiding this comment.
After switching away from root (USER ${main_user}), this layer runs apt-get update (and rosdep install typically installs apt packages). If main_user is non-root, this will fail with the same apt permissions issue the PR is trying to fix. Keep this RUN step as root (or use sudo) and only switch users after dependency installation completes.
There was a problem hiding this comment.
again, copilot is right, this part must be executed in the part where we have root permission NOT after we switch back to $main_user.
| ENV main_user=${USER} | ||
|
|
||
| USER root | ||
| RUN rm -rf /var/lib/apt/lists/* \ | ||
| && apt-get update --fix-missing \ | ||
| && apt-get -y install --no-install-recommends --fix-missing \ | ||
| ros-${ROS_DISTRO}-desktop \ | ||
| bash-completion \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY ./ /workspace/ | ||
| WORKDIR /workspace | ||
| USER ${main_user} |
There was a problem hiding this comment.
@cooperj copilot is right... we need to ensure that main_user is set to a value, so I suggest
ENV main_user=${USER:-root}so that if no $USER is defined it is set to root
| USER ${main_user} | ||
|
|
||
| WORKDIR /workspace | ||
| ENV ROS2_WS=/workspace | ||
| RUN rosdep update --rosdistro ${ROS_DISTRO} &&\ | ||
| RUN --mount=type=bind,source=.,target=/workspace \ | ||
| rosdep update --rosdistro ${ROS_DISTRO} &&\ | ||
| apt-get update && \ | ||
| rosdep install --from-paths ./src --ignore-src -r -y | ||
| rosdep install --from-paths /workspace/src --ignore-src -r -y |
There was a problem hiding this comment.
again, copilot is right, this part must be executed in the part where we have root permission NOT after we switch back to $main_user.
| RUN rm -rf /var/lib/apt/lists/* \ | ||
| && apt-get update --fix-missing \ | ||
| && apt-get -y install --no-install-recommends --fix-missing \ | ||
| ros-${ROS_DISTRO}-desktop \ |
There was a problem hiding this comment.
Why are we installing the very big desktop here? I would only install ros-${ROS_DISTRO}-base. Everything else should be handled with rosdep install below.
There was a problem hiding this comment.
ros-{distro}-desktop was originally placed here to be a placeholder for things to be installed allowing for general packages to be tested (i.e. Rviz).
-base is installed in image base image creation, would it be wiser to just not execute it here?
There was a problem hiding this comment.
ros-{distro}-desktopwas originally placed here to be a placeholder for things to be installed allowing for general packages to be tested (i.e. Rviz).
-baseis installed in image base image creation, would it be wiser to just not execute it here?
Yes, I think we don't want it here as it hides incomplete package.xml dependencies
This pull request resolves issues #17 and #18, by fixing the development container setup and its associated GitHub Actions workflow. The changes focus on improving the Dockerfile for the devcontainer and modifying the workflow configuration for building the development container image.
Devcontainer Dockerfile improvements:
rootand then reverting to the main user for subsequent commands, improving security and aligning with best practices. (.devcontainer/Dockerfile)rosdep installcommand to use a bind mount for/workspace, ensuring dependencies are installed from the correct path. (.devcontainer/Dockerfile)GitHub Actions workflow changes:
cacheFromandcacheTo) from the devcontainer build step, possibly to simplify the workflow or address caching issues. (.github/workflows/dev-container.yml).github/workflows/dev-container.yml.offto.github/workflows/dev-container.yml, re-enabling the workflow.