Skip to content
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

dev: reduce docker layers & combine install commands #992

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cstenglein
Copy link
Contributor

This reduces the layers of the DockerFile from 41 to 28.

First, I merged multiple apt-get & pip installs to one. Currently there are two apt-get installs (one with basic tools like wget & one with the rest).

Second, I merged multiple ENV & RUN statements into one but was conservative in that regard to not merge all statements, but just adjacent ones.

This should reduce the install time for the image a little bit & reduce the image size.

@benma
Copy link
Collaborator

benma commented Oct 1, 2022

Thanks.

This should reduce the install time for the image a little bit & reduce the image size.

Can you quantify that? What's the difference in size and build time roughly?

@cstenglein
Copy link
Contributor Author

cstenglein commented Oct 1, 2022

Hi @benma

Can you quantify that? What's the difference in size and build time roughly?

Sure, but it's not perfect since a big part of the image build time are the downloads.

Used time with 2 builds on my local machine to build the current master vs this branch. Note that this does only the parts of the build.sh script until docker build since docker run won't work without a firmware version.

master old
=> real 19m18.606s

Size 4.8GB

current branch
=> real 17m13.468s

Size 4.79GB

So size decrease is negligible, but build time decreased a bit (~10%).

@benma
Copy link
Collaborator

benma commented Oct 4, 2022

Have you considered moving most steps into a installation script, hence collapsing most of the layers into one?

the BitBoxApp does that:

https://github.com/digitalbitbox/bitbox-wallet-app/blob/master/Dockerfile#L21-L22

@cstenglein
Copy link
Contributor Author

Hi @benma
Sry for the late answer!

Have you considered moving most steps into a installation script, hence collapsing most of the layers into one?

Cool idea, will try that in the coming days, thanks!

@benma
Copy link
Collaborator

benma commented Nov 9, 2022

No worries, and thanks! Note that there were a lot of changes to the Dockerfile that were merged already. When you try, make sure to use the latest Dockerfile from master.

@cstenglein
Copy link
Contributor Author

@benma should work now please test :)

squashed the commits as well.

Copy link
Collaborator

@benma benma left a comment

Choose a reason for hiding this comment

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

Great work!

Could you add a small rational in the commit message body for why this is useful? As you mentioned before, a bit of reduced space and ~10% faster build time.

Comment on lines +44 to +49
doxygen \
graphviz \
python3 \
python3-pip \
clang-format-15 \
clang-tidy-15 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

These were in separate RUNs before grouped by topic (docs, CI). Would be nice to to add a comment before each group like you did below with the pip installs.

# Make Python3 the default
update-alternatives --install /usr/bin/python python /usr/bin/python3 1

# install pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Upgrade pip - it was installed before

@cstenglein cstenglein marked this pull request as draft December 3, 2022 08:04
@cstenglein
Copy link
Contributor Author

Hi @benma

Thank you for the review! Currently I'm very limited in my time so I'm converting this to a draft and reviewing it at a later time. Hope that's fine.

@benma
Copy link
Collaborator

benma commented Dec 4, 2022

@cstenglein I had actually already approved your PR, the rest were just small improvements. If you want we can merge it as is and I add these small changes on top. Let me know.

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.

2 participants