-
Notifications
You must be signed in to change notification settings - Fork 98
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
Nickez/slim down docker image #1268
base: master
Are you sure you want to change the base?
Conversation
702d3d1
to
c967beb
Compare
f80d069
to
f037d93
Compare
Python should be invoked as `python3` since we are writing python3 code and not python2. See https://peps.python.org/pep-0394/#recommendation Many (most?) distributions do not distribute a "python" executable any more, forcing you to pick either 2 or 3.
d48bff8
to
c2c9e50
Compare
Improve build/push/pull speed by reducing the number of layers and their sizes. Improve build speed by fetching pre-built binaries of cbindgen and bindgen. Silence curl progress output since it takes so many lines in CI.
c2c9e50
to
a6dff27
Compare
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.
utACK, but prefer to keep cargo install
for the two rust tools.
If your goal is to reduce layers, you could also move all of it to a docker_install.sh script and call that in a single RUN. @cstenglein has done that in this open PR #992, but somehow that PR was never merged.
COPY py/requirements.txt /tmp | ||
RUN python3 -m pip install --upgrade --requirement /tmp/requirements.txt | ||
RUN rm /tmp/requirements.txt | ||
RUN --mount=source=py,target=/mnt,rw \ |
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.
TIL
if [ "${TARGETPLATFORM}" = "linux/arm64" ]; then \ | ||
CARGO_HOME=/opt/cargo cargo install cbindgen --version ${CBINDGEN_VERSION} --locked && \ | ||
CARGO_HOME=/opt/cargo cargo install bindgen-cli --version ${BINDGEN_VERSION} --locked --target-dir=/tmp/bindgen-target && rm -r /tmp/bindgen-target; \ | ||
else \ | ||
curl -sSL https://github.com/rust-lang/rust-bindgen/releases/download/v${BINDGEN_VERSION}/bindgen-cli-x86_64-unknown-linux-gnu.tar.xz | tar -xJ --strip-components=1 -C /opt/cargo/bin bindgen-cli-x86_64-unknown-linux-gnu/bindgen && \ | ||
curl -sSL https://github.com/mozilla/cbindgen/releases/download/${CBINDGEN_VERSION}/cbindgen -o /opt/cargo/bin/cbindgen && chmod +x /opt/cargo/bin/cbindgen; \ | ||
fi && \ |
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.
imho this adds more long term maintenance effort than is worth it to have two different ways of how to get cbindgen/bindgen-cli depending on the platform.
it's not often that we need to build the image anyway, so I'd prefer to keep it simple
not a very strong opinion though, feel free to overrule :)
I have a few goals I guess:
I do think you can take the released versions of bindgen and cbindgen where available. We aren't building any other tools from source. Perhaps we could build our own linux/arm64 versions and publish somewhere in case you don't want to have two approaches in the dockerfile. The reason a reproducible container is important is so that we can build older versions of the firmware at all. Today, since the image isn't reproducible we depend on docker hub having a copy of all the versions. You can imagine some future version of a debian or pip package after some "security update" isn't compatible anymore. I think calling out to a single |
That would be nice, but I would rank this as low priority for two reasons:
|
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.
utACK
Speeds up builds for me:
What takes the most time is building bindgen and cbindgen.
What adds a lot of bloat is when things are added in one layer and then deleted in another. If you delete files in the same layer as you create them, then they are not in the final output.
Compressed sized measured with
docker save shiftcrypto/firmware_v2:44 | gzip | wc -c