Skip to content

Refactor Devcontainer Dockerfile (Root User/Bind Mounts) and Restores GitHub Actions Workflows #19

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.

Sign up for GitHub

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

Merged
cooperj merged 7 commits into from
Apr 27, 2026
Merged

Refactor Devcontainer Dockerfile (Root User/Bind Mounts) and Restores GitHub Actions Workflows #19

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
37235cf
fix: update base image
cooperj Apr 27, 2026
b6b260b
fix: restore image tag, ensure root user for apt install
cooperj Apr 27, 2026
f6719d0
fix: re-enable workflows and improve copy resolving #17
cooperj Apr 27, 2026
1e90253
fix: ensure workspace is set explicitly
cooperj Apr 27, 2026
76a1011
fix: rm caching from devcontainer ci action
cooperj Apr 27, 2026
e7fadf3
fix: move rosdep inside root session, remove desktop install
cooperj Apr 27, 2026
4d4c685
ci: force rebuild
cooperj Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions .devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,25 @@

FROM ${BASE_IMAGE} AS devcontainer

ENV main_user=${USER}

Check warning on line 6 in .devcontainer/Dockerfile

View workflow job for this annotation

GitHub Actions / build

Variables should be defined before their use 

UndefinedVar: Usage of undefined variable '$USER'
More info: https://docs.docker.com/go/dockerfile/rule/undefined-var/

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 \

@marc-hanheide marc-hanheide Apr 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cooperj cooperj Apr 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@marc-hanheide marc-hanheide Apr 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Yes, I think we don't want it here as it hides incomplete package.xml dependencies

bash-completion \
&& rm -rf /var/lib/apt/lists/*

COPY ./ /workspace/
WORKDIR /workspace
USER ${main_user}

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

@marc-hanheide marc-hanheide Apr 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


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

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

@marc-hanheide marc-hanheide Apr 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



# Do an initial build to speed up development
RUN /bin/bash -lc "source /opt/ros/${ROS_DISTRO}/setup.bash && colcon build --continue-on-error"
Expand Down
0 .github/workflows/build-docker.yml.off → .github/workflows/build-docker.yml
View file
Open in desktop
File renamed without changes.
2 changes: 0 additions & 2 deletions .github/workflows/dev-container.yml.off → .github/workflows/dev-container.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,3 @@ jobs:
imageName: devcontainer/${{ steps.docker_image_name.outputs.docker_image }}
configFile: ./.devcontainer/devcontainer.json
push: never
cacheFrom: type=gha
Comment thread
marc-hanheide marked this conversation as resolved.
cacheTo: type=gha,mode=max
0 .github/workflows/ros-ci.yml.off → .github/workflows/ros-ci.yml
View file
Open in desktop
File renamed without changes.
Loading