-
Notifications
You must be signed in to change notification settings - Fork 16
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
Update dockerfiles #515
Update dockerfiles #515
Conversation
RUN apt-get update && \ | ||
DEBIAN_FRONTEND=noninteractive \ | ||
apt-get install -y --no-install-recommends \ | ||
autoconf automake autotools-dev curl python3 bc lsb-release \ | ||
autoconf automake autotools-dev curl python3.11 python3.11-venv python3-pip bc lsb-release \ | ||
libmpc-dev libmpfr-dev libgmp-dev gawk build-essential \ | ||
bison flex texinfo gperf libtool patchutils zlib1g-dev \ | ||
libexpat-dev ninja-build git ca-certificates python-is-python3 && \ | ||
bison flex texinfo gperf libtool patchutils zlib1g-dev device-tree-compiler \ | ||
libexpat-dev ninja-build git ca-certificates python-is-python3 \ | ||
libssl-dev libbz2-dev libreadline-dev libsqlite3-dev libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev && \ |
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.
Regarding your comment #513 (comment) and this change - I don't think that running apt-get
only in the first RUN
is that better. Opposite has its benefits - if I wanted to add another tool like spike, with its dependencies I only created next layer that is entirely dedicated to the tool. I didn't modify previous layers, so I didn't need to rebuild the entire container (cached versions of first two steps were used) or upload first two layers again. I would be for this option, as it is useful for updating and developing containers.
Are there some benefits of installing everything in first step, that I am missing?
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 is a benefit that you don't call apt update
multiple times, so you generate lower traffic for Debian servers.
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.
But on the other hand, when your are adding new tool with new package dependencies as a new step, instead of installing all of the packages again at rebuild, you use cached previous docker layers and only download the newly added packages, which creates even less traffic.
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.
Note that there is rm -rf /var/lib/apt/lists/*
in the end of the command. This will make the resulting docker image smaller (by noticeable amount), as all the files that we are going to delete will be ephemeral (and won't exist in the resulting image). If we split that into two RUN
instructions, the contents of /var/lib/apt/lists/
would be saved as part of the first layer (each instruction is a new layer).
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.
Okay, lets merge it. I will merge it to my PR after this one is on master
docker/riscv-toolchain.Dockerfile
Outdated
eval "$(pyenv init --path)" && \ | ||
pyenv install 3.6.15 && \ | ||
pyenv global 3.6.15 && \ | ||
python -V && \ |
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.
please remove this line
While updating #454 I have found that we don't have venv in dockers, so it have to be manually installed before running the regression tests by hand. Here is a PR which update dockers to contain this package. Additionally I integrated changes added by @piotro888 in #513 to update dockers only once.
(Dockers are being currently uploaded)