Use bind mounts in Docker builds (#951 follow-up)#958
Conversation
7fb56d0 to
61869bc
Compare
0b84c58 to
3587fd9
Compare
c6ea361 to
c29b53b
Compare
This is to address an earlier review comment on #951: #951 (comment) From https://docs.docker.com/engine/storage/bind-mounts: > Bind mounts are also available for builds: you can bind mount source > code from the host into the build container to test, lint, or compile > a project. The advantages of: ``` RUN --mount=type=bind,dst=/mnt /mnt/setup/script.sh ``` ... over: ``` COPY /setup/script.sh / RUN /script.sh ``` ... are that: - fewer layers are built, (~`COPY`~) - no layer is bloated by build-only files, - the filesystem's root is kept in a cleaner state at every stage, - repeated invocations over `Dockerfile`s are limited to single RUN lines. ([DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself)) Some more refs: - moby/buildkit#2893 - https://stackoverflow.com/questions/72882140/docker-buildkit-new-mount-command-during-build-and-large-files-in-the-context - https://www.bestonlinetutorial.com/docker/how-can-you-mount-host-volumes-into-docker-containers-in-a-dockerfile-during-build.html
c29b53b to
51de9d4
Compare
bind mounts for builds (#951 follow-up)
bind mounts for builds (#951 follow-up)bind mounts in builds (#951 follow-up)
bind mounts in builds (#951 follow-up)bind mounts in Docker builds (#951 follow-up)
KevinFairise2
left a comment
There was a problem hiding this comment.
TIL about RUN --mount=type=bind !
ofek
left a comment
There was a problem hiding this comment.
This is much cleaner, thanks!
| mkdir ~/.scripts | ||
| ( | ||
| cd "$(dirname "${BASH_SOURCE[0]}")"/scripts | ||
| for f in *.sh; do | ||
| install -m 755 "$f" ~/.scripts/"${f%.sh}" | ||
| done | ||
| ) | ||
| ~/.scripts/path-prepend ~/.scripts |
There was a problem hiding this comment.
Just a nit for consistency with other files used to set up the developer environment, and for correctness in case the home directory contains spaces. Although I know that won't happen in practice, linters might check this.
| mkdir ~/.scripts | |
| ( | |
| cd "$(dirname "${BASH_SOURCE[0]}")"/scripts | |
| for f in *.sh; do | |
| install -m 755 "$f" ~/.scripts/"${f%.sh}" | |
| done | |
| ) | |
| ~/.scripts/path-prepend ~/.scripts | |
| mkdir "${HOME}/.scripts" | |
| ( | |
| cd "$(dirname "${BASH_SOURCE[0]}")"/scripts | |
| for f in *.sh; do | |
| install -m 755 "$f" "${HOME}/.scripts/${f%.sh}" | |
| done | |
| ) | |
| "${HOME}/.scripts/path-prepend" "${HOME}/.scripts" |
There was a problem hiding this comment.
[...] for correctness in case the home directory contains spaces.
I've been actually concluding the opposite: ~ turns out to be well-suited to sidestep quoting/splitting pitfalls while giving a concise, idiomatic, and unambiguous way to refer to the user’s home directory.
In Bash and POSIX-compliant shells, tilde expansion is treated as if the result were quoted. This means that the expanded value is not subject to further word splitting nor filename expansion/globbing.
According to POSIX Shell, 2.6.1 Tilde Expansion:
The pathname resulting from tilde expansion shall be treated as if quoted to prevent it being altered by field splitting and pathname expansion.
And to GNU Bash, 3.5.2 Tilde Expansion:
The results of tilde expansion are treated as if they were quoted, so the replacement is not subject to word splitting and filename expansion.
This behavior is also important for security and predictability when working with file paths and variable assignments, but I had to admit I don't find a link to the manual that used to enlighten me - probably a chapter of the The Art of Unix Programming.
[...] linters might check this.
Good linters such as shellcheck actually do check this.
[...] for consistency with other files used to set up the developer environment [...]
That said, I’ll defer and go with this so we can keep things moving.
There was a problem hiding this comment.
✅ Done in 3rd commit.
This is to address 3 related review comments at once: - #958 (comment) - #958 (comment) - #958 (comment) It consists in moving the Zsh setup close to its Nushell peer, that is prior to installing helper scripts.
This is to comply with #958 (comment), as it turns out local code owners prefer using "$HOME" over ~, despite the guaranteed tilde expansion in POSIX Shells: - https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_01 - https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html#Tilde-Expansion-1
This is to address 3 related review comments at once: - #958 (comment) - #958 (comment) - #958 (comment) It consists in moving the Zsh setup close to its Nushell peer, that is prior to installing helper scripts.
This is to comply with #958 (comment), as it turns out local code owners prefer using "$HOME" over ~, despite the guaranteed tilde expansion in POSIX Shells: - https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_01 - https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html#Tilde-Expansion-1
|
Thanks for the reviews, guys! |
It turns out `bind` mounts introduced by #958 in Linux Docker builds also work well in Windows builds. The present change therefore proposes to leverage the feature here too so as to shrink down layers affected by repeated `COPY`s. (overlays) Doing so unearthed a few issues: (thanks to `bind` mounts in builds being read/only by design) - quite a number of temporary files (installers, tarballs) where downloaded/extracted to various depths within installation scripts, - some of these temporaries were not removed after installation: - under `./`: `get-pip.py`, (first occurrence) - under `./helpers/phase1`: `wdksetup.exe`, - under `./helpers/phase2`: `get-pip.py`, (second occurrence) - under `./helpers/phase3`: `rustup-init.exe`. The present change therefore addresses these as well.
It turns out `bind` mounts introduced by #958 in Linux Docker builds also work well in Windows builds. The present change therefore proposes to leverage the feature here too so as to shrink down layers affected by repeated `COPY`s. (overlays) Doing so unearthed a few issues: (thanks to `bind` mounts in builds being read/only by design) - quite a number of temporary files (installers, tarballs) where downloaded/extracted to various depths within installation scripts, - some of these temporaries were not removed after installation: - under `./`: `get-pip.py`, (first occurrence) - under `./helpers/phase1`: `wdksetup.exe`, - under `./helpers/phase2`: `get-pip.py`, (second occurrence) - under `./helpers/phase3`: `rustup-init.exe`. The present change therefore addresses these as well.
It turns out `bind` mounts introduced by #958 in Linux Docker builds also work well in Windows builds. The present change therefore proposes to leverage the feature here too so as to shrink down layers affected by repeated `COPY`s. (overlays) Doing so unearthed a few issues: (thanks to `bind` mounts in builds being read/only by design) - quite a number of temporary files (installers, tarballs) where downloaded/extracted to various depths within installation scripts, - some of these temporaries were not removed after installation: - under `./`: `get-pip.py`, (first occurrence) - under `./helpers/phase1`: `wdksetup.exe`, - under `./helpers/phase2`: `get-pip.py`, (second occurrence) - under `./helpers/phase3`: `rustup-init.exe`. The present change therefore addresses these as well.
It turns out `bind` mounts introduced by #958 in Linux Docker builds also work well in Windows builds. The present change therefore proposes to leverage the feature here too so as to shrink down layers affected by repeated `COPY`s. (overlays) Doing so unearthed a few issues: (thanks to `bind` mounts in builds being read/only by design) - quite a number of temporary files (installers, tarballs) where downloaded/extracted to various depths within installation scripts, - some of these temporaries were not removed after installation: - under `./`: `get-pip.py`, (first occurrence) - under `./helpers/phase1`: `wdksetup.exe`, - under `./helpers/phase2`: `get-pip.py`, (second occurrence) - under `./helpers/phase3`: `rustup-init.exe`. The present change therefore addresses these as well.
* Also use `bind` mounts in Windows Docker builds It turns out `bind` mounts introduced by #958 in Linux Docker builds also work well in Windows builds. The present change therefore proposes to leverage the feature here too so as to shrink down layers affected by repeated `COPY`s. (overlays) Doing so unearthed a few issues: (thanks to `bind` mounts in builds being read/only by design) - quite a number of temporary files (installers, tarballs) where downloaded/extracted to various depths within installation scripts, - some of these temporaries were not removed after installation: - under `./`: `get-pip.py`, (first occurrence) - under `./helpers/phase1`: `wdksetup.exe`, - under `./helpers/phase2`: `get-pip.py`, (second occurrence) - under `./helpers/phase3`: `rustup-init.exe`. The present change therefore addresses these as well. * Map `WORKDIR` to a meaningful directory This is to address following review comment: > Might be better to use Windows-style paths here, maybe `c:/mnt` ? (#963 (comment)) The idea here is to map WORKDIR to a well-known, existing root directory (the default one under Windows). For good measure, the earlier mount point gets also deleted.
### What does this PR do? Replaces all `RUN --mount` instructions in the Dockerfiles to specifically mount the files it needs instead of the entire repo. Also moves a bunch of logic from the dockerfile itself into separate scripts for better readability and maintainability. ### Motivation Mounting the entire repo _destroys_ caching performance: with the entire repo mounted, the cached value of each layer is invalidated as soon as _any_ file in the repo gets changed. By mounting only the necessary file(s) in each `RUN` layer, we make sure that these layer's cached values are invalidated if and only if these specific files get changed, instead of any file in the repo. ### Possible Drawbacks / Trade-offs None, as long as every image builds properly. Maybe it's a bit more verbose. ### Additional Notes See #958. I did not make the change for the Windows image as it requires mounting a lot more files - I will probably do it in a follow-up PR. Tested in DataDog/datadog-agent#41927
What does this PR do?
This change introduces
bindmounts in Docker builds for Linux. A later PR might be filed for Windows (#963).This is to address an earlier review comment on #951:
Motivation
From https://docs.docker.com/engine/storage/bind-mounts:
The advantages of:
... over:
... are that:
)COPYDockerfiles are limited to single RUN lines.Additional Notes
💡 For
dev-envs/linux, I double-checked that we get the very same$PATHas prior to the change - that is for all of:~/.bashrc~/.config/nushell/env.nu($NUSHELL_ENV_FILE)~/.profile~/.zshenvSome more refs: