Skip to content

Use bind mounts in Docker builds (#951 follow-up)#958

Merged
rdesgroppes merged 3 commits intomainfrom
regis.desgroppes/use-docker-bind-mounts-for-builds
Aug 19, 2025
Merged

Use bind mounts in Docker builds (#951 follow-up)#958
rdesgroppes merged 3 commits intomainfrom
regis.desgroppes/use-docker-bind-mounts-for-builds

Conversation

@rdesgroppes
Copy link
Copy Markdown
Contributor

@rdesgroppes rdesgroppes commented Aug 18, 2025

What does this PR do?

This change introduces bind mounts in Docker builds for Linux. A later PR might be filed for Windows (#963).

This is to address an earlier review comment on #951:

[...] Since the block is the same across several different Dockerfiles I would rather reuse a shell script and do something like:
RUN --mount=type=bind,src=install_bazelisk.sh,target=/tmp/install_bazelisk.sh \ chmod +x /tmp/install_bazelisk.sh && /tmp/install_bazelisk.sh

This way you don't need to COPY that script it into the layer

Motivation

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 with build-only files,
  • the filesystem's root is kept in a cleaner state at every stage,
  • repeated invocations over Dockerfiles are limited to single RUN lines.

Additional Notes

💡 For dev-envs/linux, I double-checked that we get the very same $PATH as prior to the change - that is for all of:

  • ~/.bashrc
  • ~/.config/nushell/env.nu ($NUSHELL_ENV_FILE)
  • ~/.profile
  • ~/.zshenv

Some more refs:

@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/use-docker-bind-mounts-for-builds branch from 7fb56d0 to 61869bc Compare August 18, 2025 16:18
@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/use-docker-bind-mounts-for-builds branch 2 times, most recently from 0b84c58 to 3587fd9 Compare August 18, 2025 18:18
@rdesgroppes rdesgroppes changed the title [WIP] Use docker bind mounts for builds [WIP] Use docker bind mounts for builds (#951 follow-up) Aug 18, 2025
@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/use-docker-bind-mounts-for-builds branch 11 times, most recently from c6ea361 to c29b53b Compare August 19, 2025 12:51
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
@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/use-docker-bind-mounts-for-builds branch from c29b53b to 51de9d4 Compare August 19, 2025 13:24
@rdesgroppes rdesgroppes changed the title [WIP] Use docker bind mounts for builds (#951 follow-up) Use Docker bind mounts for builds (#951 follow-up) Aug 19, 2025
@rdesgroppes rdesgroppes marked this pull request as ready for review August 19, 2025 13:33
@rdesgroppes rdesgroppes requested review from a team as code owners August 19, 2025 13:33
@rdesgroppes rdesgroppes changed the title Use Docker bind mounts for builds (#951 follow-up) Use Docker bind mounts in builds (#951 follow-up) Aug 19, 2025
@rdesgroppes rdesgroppes changed the title Use Docker bind mounts in builds (#951 follow-up) Use bind mounts in Docker builds (#951 follow-up) Aug 19, 2025
Copy link
Copy Markdown
Member

@KevinFairise2 KevinFairise2 left a comment

Choose a reason for hiding this comment

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

TIL about RUN --mount=type=bind !

Copy link
Copy Markdown

@gabedos gabedos left a comment

Choose a reason for hiding this comment

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

refactor lgtm

Copy link
Copy Markdown
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

This is much cleaner, thanks!

Comment thread dev-envs/linux/Dockerfile Outdated
Comment thread dev-envs/linux/Dockerfile Outdated
Comment thread dev-envs/linux/Dockerfile Outdated
Comment thread dev-envs/linux/scripts.sh Outdated
Comment on lines +5 to +12
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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"

Copy link
Copy Markdown
Contributor Author

@rdesgroppes rdesgroppes Aug 19, 2025

Choose a reason for hiding this comment

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

[...] 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

✅ Done in 3rd commit.

rdesgroppes added a commit that referenced this pull request Aug 19, 2025
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.
rdesgroppes added a commit that referenced this pull request Aug 19, 2025
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
@rdesgroppes rdesgroppes requested a review from ofek August 19, 2025 21:52
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
@rdesgroppes
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews, guys!

@rdesgroppes rdesgroppes merged commit 3b75f63 into main Aug 19, 2025
109 of 111 checks passed
@rdesgroppes rdesgroppes deleted the regis.desgroppes/use-docker-bind-mounts-for-builds branch August 19, 2025 22:57
rdesgroppes added a commit that referenced this pull request Aug 21, 2025
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.
rdesgroppes added a commit that referenced this pull request Aug 21, 2025
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.
rdesgroppes added a commit that referenced this pull request Aug 21, 2025
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.
rdesgroppes added a commit that referenced this pull request Aug 21, 2025
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.
rdesgroppes added a commit that referenced this pull request Aug 21, 2025
* 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.
Ishirui added a commit that referenced this pull request Dec 8, 2025
### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants