search

BitBoxSwiss / bitbox02-firmware

Firmware code of the BitBox02 hardware wallet
https://bitbox.swiss/bitbox02
Apache License 2.0
263 stars 95 forks source link

dev: reduce docker layers & combine install commands #992

Open cstenglein opened 2 years ago

cstenglein commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@benma should work now please test :)

squashed the commits as well.

cstenglein commented 1 year ago

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 commented 1 year ago

@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.