Skip to content

[ROS] Update Official Docker image docs #2578

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

Merged
merged 32 commits into from
Jun 20, 2025
Merged

Conversation

ruffsl
Copy link
Contributor

@ruffsl ruffsl commented May 29, 2025

Refreshes the ROS documentation using updated links, cleans out old content, and improves the demo examples.

Related:

ruffsl added 28 commits May 28, 2025 13:42
as final ROS 1 version, Noetic, is now EOL
as docs page is more up-to-date and pertinent for users
to avoid docs from becoming stale
as example to save on image size
as well as on build time using caching
for readability of each stage
and update rosdep along with apt list
to ensure package index is fresh
to include additional relevant entries
to focus on primary targets
accounting for included docker-clean default
to avoid running rosdep update for each source change
as docker build --no-cache can still be used to force update
@ruffsl
Copy link
Contributor Author

ruffsl commented May 30, 2025

@mikaelarguedas or @tfoote , could you take a look this? Think it's about ready to go. Thanks!

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

It generally looks like a good improvement. The naming of installer versus runner seem a little bit confusing to me. I feel like installer might be clearer with a name more like "upstream" or "prebuilt" maybe.

@mikaelarguedas
Copy link
Contributor

looks great overall !

agreed with Tully's comment

maybe using latest instead of rolling for the base images would be as future-proof and give example of using stable distros

while keeping with uniquely spelled noun
that is self descriptive yet unmistakable
@ruffsl
Copy link
Contributor Author

ruffsl commented Jun 2, 2025

maybe using latest instead of rolling for the base images would be as future-proof and give example of using stable distros

For reproducibility, I'd generally like to discourage implicit or explicit use of latest in favor of a distro specific tag. If any novice user copies an example using latest, chances are it'll break their Dockerfile a much later, catching them off guard once a new stable distro is release, where as if they copied from the example using rolling, it's more apparent that it a rolling distribution and if they want swap the tag for something else. However, if that kind of breakage is their intent, e.g. for CI tracking latest LTS, then then can conscientiously make that choice, setting the tag to latest instead.

Also, this exemplifies how to postfix an ARG to minimize the runtime image, and ros:latest-ros-core is non-existent.

FROM $FROM_IMAGE-ros-core AS runner

@ruffsl ruffsl marked this pull request as ready for review June 2, 2025 22:52
@ruffsl
Copy link
Contributor Author

ruffsl commented Jun 19, 2025

@tianon or @yosifkit , could we get this upstreamed? The docs could really use an update to match recent changes.

@tianon
Copy link
Member

tianon commented Jun 20, 2025

Oof, sorry, I was under the assumption it was still a WIP because GitHub doesn't send an email for "ruffsl marked this pull request as ready for review 3 weeks ago" 🙃 🙇

Will review ASAP.

Comment on lines +56 to +59
cat <<EOF > /etc/apt/apt.conf.d/docker-clean && apt-get update
APT::Install-Recommends "false";
APT::Install-Suggests "false";
EOF
Copy link
Member

Choose a reason for hiding this comment

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

This file (/etc/apt/apt.conf.d/docker-clean) already exists in the image -- did you overwrite that content on purpose, or did you intend for this to go to a different filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the overwrite was intentional to remove the prior rule that cleans the deb cache after dpkg is invoked.

It might be a too cute, but wanted to keep the example succinct , avoiding unnecessary rm or mv commands.

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't immediately clear to me why you'd want to remove that configuration -- maybe it warrants a comment that explains why / that this is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +117 to +118
COPY --from=cacher /tmp/exec_debs.txt /tmp/exec_debs.txt
RUN --mount=type=cache,target=/etc/apt/apt.conf.d,from=cacher,source=/etc/apt/apt.conf.d \
Copy link
Member

Choose a reason for hiding this comment

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

You could do this in the builder stage too, but since this is the final stage it might be worth adding exec_debs.txt to a --mount instead so the file doesn't end up in the final image.

Suggested change
COPY --from=cacher /tmp/exec_debs.txt /tmp/exec_debs.txt
RUN --mount=type=cache,target=/etc/apt/apt.conf.d,from=cacher,source=/etc/apt/apt.conf.d \
RUN --mount=type=bind,from=cacher,source=/tmp/exec_debs.txt,target=/tmp/exec_debs.txt \
--mount=type=cache,target=/etc/apt/apt.conf.d,from=cacher,source=/etc/apt/apt.conf.d \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about doing that, but figured including the derived list of runtime dependencies in the output image could be more helpful for traceability and transparent debugging of the same layerer downstream. Users could of course docker build --target=cacher ... and inspect that intermediate image, but wouldn't propagate receipts.

@tianon tianon merged commit 2b841c4 into docker-library:master Jun 20, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants