search

Tiryoh / docker-ros2-desktop-vnc

🐳 Dockerfiles to provide HTML5 VNC interface to access Ubuntu Desktop + ROS 2
https://memoteki.net/archives/2955
Apache License 2.0
408 stars 82 forks source link

Why `rm /etc/apt/apt.conf.d/docker-clean` is needed? #152

Closed takasehideki closed 4 months ago

takasehideki commented 5 months ago

This may not be a suggestion but a question.

I came across the following line and comment and wondered why they were added and how it works. https://github.com/Tiryoh/docker-ros2-desktop-vnc/blob/master/humble/Dockerfile#L64-L65

The RUN line seems to have been added with #79 (file) and the relevant comment with #92 (file).

According to these web pages, /etc/apt/apt.conf.d/ contributes to faster builds, and removing it may increase the image size. https://dev.classmethod.jp/articles/apt-get-magic-spell-in-docker/ https://askubuntu.com/questions/735189/enabling-auto-completion-for-apt-get-install-in-docker-ubuntu-14-04

When tested on an arm64 PC (MacBook), removing this RUN line does indeed significantly reduce the image size (-1 is the RUN line removed).

% docker images
REPOSITORY                      TAG                                            IMAGE ID       CREATED        SIZE
tiryoh/ros2-desktop-vnc-1       humble                                         87ddf8e30560   2 hours ago    6.33GB
tiryoh/ros2-desktop-vnc         humble                                         c5cb1a2daf3c   2 hours ago    7.28GB

I briefly checked the built image with this and it does not seem to have a significant effect on the operation of its container. My concern is in why you have added this line or thought it is necessary. At least the comment doesn't seem to make sense,,, but if you have a sound reason to do so, I'm fine with it as it is (because the image size is large to begin with :D

If you don't mind, I would be happy to send you a PR about this (as I am already prepared to do so). https://github.com/takasehideki/docker-ros2-desktop-vnc/tree/rm_docker-clean https://github.com/takasehideki/docker-ros2-desktop-vnc/commit/6384312ac3483c5fe5b6a033fa55ca3a9a14cbab

Tiryoh commented 5 months ago

Hi @takasehideki,

Thank you for the feedback. I welcome your proposed pull request.

For the record, I'll answer your question. The purpose of the command rm /etc/apt/apt.conf.d/docker-clean is to enable auto-completion with the apt install command inside the container's bash. There's no profound reason for the comments in the Dockerfile being added in a separate commit, though they should ideally have been included in the same commit.

Leaving the docker-clean file results in the inability to use auto-completion with the apt install command after running apt update inside the container. This Dockerfile is designed for setting up and using a test environment, and having bash auto-completion available inside the container was considered beneficial for both me and the users.

However, I had not fully taken into account the impact on the image size. I agree that keeping the image size as small as possible is desirable. For now, I'll work on reducing the image size. Afterward, I'm thinking of considering adding rm /etc/apt/apt.conf.d/docker-clean to entrypoint.sh or something similar.

takasehideki commented 5 months ago

@Tiryoh thank you so much for your kind explanation! First, in my conclusion, the command RUN rm /etc/apt/apt.conf.d/docker-clean should be moved to the later stage of the Dockerfile. I hope you like my PR #153 about this.

It would certainly be useful to realize apt-get auto-completion. My first suggestion of just removing this command would not allow the auto-completion even after doing apt update. According to this page, it seems that docker-clean should be removed to realize apt-get auto-completion. By moving this command to the later, I confirmed that the auto-completion could be kept after running apt update in the container, while greatly reducing the size of the Docker image (6.33 GB as indicated in my initial proposal). One strange thing is that auto-completion in apt install was enabled in the original Docker image without doing apt update. This feature is lost by moving the command with my commit. However, I don't think it's a matter, because we can't operate apt install without doing apt update in the first place, and it's natural to operate apt update first if we want to install a new package in a Docker container.