| Message ID | 20240713144316.403640-1-arnout@mind.be |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] utils/docker-run: allow to specify extra mount points to propagate | expand |
Hi Arnout, On Sat Jul 13, 2024 at 2:43 PM UTC, Arnout Vandecappelle via buildroot wrote: > Sometimes, the build needs access to directories which are outside of > the current directory, e.g. for pre-downloaded toolchains, local kernel > sources, OVERRIDE_SRCDIR, BR2_EXTERNALs, ... We need these to be mounted > into the container to be able to perform the build. > > Since there is no generic way to find out all the directories that are > needed, we need a manual mechanism. We choose the environment variable > EXTRA_MOUNTPOINTS which contains a space-separated list of directories > (or files) to mount. > > We choose an environment variable to avoid having to parse command-line > arguments to docker-run. > > Update the terse documentation in utils/readme.txt with this > information. > > Signed-off-by: Arnout Vandecappelle <arnout@mind.be> > --- > utils/docker-run | 5 +++++ > utils/readme.txt | 5 ++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/utils/docker-run b/utils/docker-run > index 1adb02d74e..3bb7b6a41b 100755 > --- a/utils/docker-run > +++ b/utils/docker-run > @@ -90,6 +90,11 @@ if [ "${BR2_DL_DIR}" ]; then > docker_opts+=( --env BR2_DL_DIR ) > fi > > +if [ -n "${EXTRA_MOUNTPOINTS:-}" ]; then > + read -r -a extra_mountpoints <<<"${EXTRA_MOUNTPOINTS}" I think it would be good to use `IFS=:` here and split EXTRA_MOUNTPOINTS on ':'. That would make sure we don't split on other unexpected characters like spaces, tabs, and newlines. And it would be consistent with how BR2_EXTERNAL works. Thanks, Brandon > + mountpoints+=( "${extra_mountpoints[@]}" ) > +fi > + > # shellcheck disable=SC2013 # can't use while-read because of the assignment > for dir in $(printf '%s\n' "${mountpoints[@]}" |LC_ALL=C sort -u); do > docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" ) > diff --git a/utils/readme.txt b/utils/readme.txt > index 6488d13c75..0cc665e478 100644 > --- a/utils/readme.txt > +++ b/utils/readme.txt > @@ -21,7 +21,10 @@ check-package > docker-run > a script that runs a command (like make check-package) inside the > buildroot CI docker container; pass no command to get an interactive > - shell. > + shell. If additional directories need to be accessible inside the > + container, specify them with the environment variable EXTRA_MOUNTPOINTS. > + The buildroot directory, the current directory, and the download > + directory are automatically propagated. > > genrandconfig > a script that generates a random configuration, used by the autobuilders
On 13/07/2024 18:24, Brandon Maier wrote: > Hi Arnout, > > On Sat Jul 13, 2024 at 2:43 PM UTC, Arnout Vandecappelle via buildroot wrote: >> Sometimes, the build needs access to directories which are outside of >> the current directory, e.g. for pre-downloaded toolchains, local kernel >> sources, OVERRIDE_SRCDIR, BR2_EXTERNALs, ... We need these to be mounted >> into the container to be able to perform the build. >> >> Since there is no generic way to find out all the directories that are >> needed, we need a manual mechanism. We choose the environment variable >> EXTRA_MOUNTPOINTS which contains a space-separated list of directories >> (or files) to mount. >> >> We choose an environment variable to avoid having to parse command-line >> arguments to docker-run. >> >> Update the terse documentation in utils/readme.txt with this >> information. >> >> Signed-off-by: Arnout Vandecappelle <arnout@mind.be> >> --- >> utils/docker-run | 5 +++++ >> utils/readme.txt | 5 ++++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/utils/docker-run b/utils/docker-run >> index 1adb02d74e..3bb7b6a41b 100755 >> --- a/utils/docker-run >> +++ b/utils/docker-run >> @@ -90,6 +90,11 @@ if [ "${BR2_DL_DIR}" ]; then >> docker_opts+=( --env BR2_DL_DIR ) >> fi >> >> +if [ -n "${EXTRA_MOUNTPOINTS:-}" ]; then >> + read -r -a extra_mountpoints <<<"${EXTRA_MOUNTPOINTS}" > > I think it would be good to use `IFS=:` here and split EXTRA_MOUNTPOINTS > on ':'. That would make sure we don't split on other unexpected > characters like spaces, tabs, and newlines. And it would be consistent > with how BR2_EXTERNAL works. Well, there's a bigger chance that a file/directory contains a colon than that it contains a newline or tab, so : is not _that_ much better. Between tab and space I'd say they are about equally likely. However, a path with a space will simply not be supported by Buildroot. So if that extra mountpoint has a space and is used for a BR2_EXTERNAL, for a local tarball, for an override srcdir, for an output directory, for a host directory, for a download directory, or for some more things I'm forgetting, it simply will not work. On the other hand, a path that has a colon in it _can_ be used for all of these things, except for a BR2_EXTERNAL. So I'm no fan of using a colon. Yann typically has a useful opinion about this kind of thing, so I'm putting him in Cc. Regards, Arnout > > Thanks, > Brandon > >> + mountpoints+=( "${extra_mountpoints[@]}" ) >> +fi >> + >> # shellcheck disable=SC2013 # can't use while-read because of the assignment >> for dir in $(printf '%s\n' "${mountpoints[@]}" |LC_ALL=C sort -u); do >> docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" ) >> diff --git a/utils/readme.txt b/utils/readme.txt >> index 6488d13c75..0cc665e478 100644 >> --- a/utils/readme.txt >> +++ b/utils/readme.txt >> @@ -21,7 +21,10 @@ check-package >> docker-run >> a script that runs a command (like make check-package) inside the >> buildroot CI docker container; pass no command to get an interactive >> - shell. >> + shell. If additional directories need to be accessible inside the >> + container, specify them with the environment variable EXTRA_MOUNTPOINTS. >> + The buildroot directory, the current directory, and the download >> + directory are automatically propagated. >> >> genrandconfig >> a script that generates a random configuration, used by the autobuilders
On Sat Jul 13, 2024 at 4:30 PM UTC, Arnout Vandecappelle via buildroot wrote: > > > On 13/07/2024 18:24, Brandon Maier wrote: > > Hi Arnout, > > > > On Sat Jul 13, 2024 at 2:43 PM UTC, Arnout Vandecappelle via buildroot wrote: > >> Sometimes, the build needs access to directories which are outside of > >> the current directory, e.g. for pre-downloaded toolchains, local kernel > >> sources, OVERRIDE_SRCDIR, BR2_EXTERNALs, ... We need these to be mounted > >> into the container to be able to perform the build. > >> > >> Since there is no generic way to find out all the directories that are > >> needed, we need a manual mechanism. We choose the environment variable > >> EXTRA_MOUNTPOINTS which contains a space-separated list of directories > >> (or files) to mount. > >> > >> We choose an environment variable to avoid having to parse command-line > >> arguments to docker-run. > >> > >> Update the terse documentation in utils/readme.txt with this > >> information. > >> > >> Signed-off-by: Arnout Vandecappelle <arnout@mind.be> > >> --- > >> utils/docker-run | 5 +++++ > >> utils/readme.txt | 5 ++++- > >> 2 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/utils/docker-run b/utils/docker-run > >> index 1adb02d74e..3bb7b6a41b 100755 > >> --- a/utils/docker-run > >> +++ b/utils/docker-run > >> @@ -90,6 +90,11 @@ if [ "${BR2_DL_DIR}" ]; then > >> docker_opts+=( --env BR2_DL_DIR ) > >> fi > >> > >> +if [ -n "${EXTRA_MOUNTPOINTS:-}" ]; then > >> + read -r -a extra_mountpoints <<<"${EXTRA_MOUNTPOINTS}" > > > > I think it would be good to use `IFS=:` here and split EXTRA_MOUNTPOINTS > > on ':'. That would make sure we don't split on other unexpected > > characters like spaces, tabs, and newlines. And it would be consistent > > with how BR2_EXTERNAL works. > > Well, there's a bigger chance that a file/directory contains a colon than that > it contains a newline or tab, so : is not _that_ much better. Between tab and > space I'd say they are about equally likely. > > However, a path with a space will simply not be supported by Buildroot. So if > that extra mountpoint has a space and is used for a BR2_EXTERNAL, for a local > tarball, for an override srcdir, for an output directory, for a host directory, > for a download directory, or for some more things I'm forgetting, it simply will > not work. > > On the other hand, a path that has a colon in it _can_ be used for all of > these things, except for a BR2_EXTERNAL. > > So I'm no fan of using a colon. Yes that's a good point, it would be preferable to allow any characters in a mount path, and a colon is probably more common then whitespace. One thing I also thought of, a colon would be consistent not only with $BR2_EXTERNAL, but many common env variables use colons to seperate paths. E.g. $PATH, $LD_LIBRARY_PATH, $MANPATH, etc. I'm assuming that was why $BR2_EXTERNAL chose to use colons as well. But this is a policy decision, so whichever one is used the code itself look good. Reviewed-by: Brandon Maier <brandon.maier@collins.com> Thanks, Brandon > > Yann typically has a useful opinion about this kind of thing, so I'm putting > him in Cc. > > Regards, > Arnout > > > > > > Thanks, > > Brandon > > > >> + mountpoints+=( "${extra_mountpoints[@]}" ) > >> +fi > >> + > >> # shellcheck disable=SC2013 # can't use while-read because of the assignment > >> for dir in $(printf '%s\n' "${mountpoints[@]}" |LC_ALL=C sort -u); do > >> docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" ) > >> diff --git a/utils/readme.txt b/utils/readme.txt > >> index 6488d13c75..0cc665e478 100644 > >> --- a/utils/readme.txt > >> +++ b/utils/readme.txt > >> @@ -21,7 +21,10 @@ check-package > >> docker-run > >> a script that runs a command (like make check-package) inside the > >> buildroot CI docker container; pass no command to get an interactive > >> - shell. > >> + shell. If additional directories need to be accessible inside the > >> + container, specify them with the environment variable EXTRA_MOUNTPOINTS. > >> + The buildroot directory, the current directory, and the download > >> + directory are automatically propagated. > >> > >> genrandconfig > >> a script that generates a random configuration, used by the autobuilders > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Am 13.07.24 um 21:48 schrieb Brandon Maier: > On Sat Jul 13, 2024 at 4:30 PM UTC, Arnout Vandecappelle via buildroot wrote: >> >> >> On 13/07/2024 18:24, Brandon Maier wrote: >>> Hi Arnout, >>> >>> On Sat Jul 13, 2024 at 2:43 PM UTC, Arnout Vandecappelle via buildroot wrote: >>>> Sometimes, the build needs access to directories which are outside of >>>> the current directory, e.g. for pre-downloaded toolchains, local kernel >>>> sources, OVERRIDE_SRCDIR, BR2_EXTERNALs, ... We need these to be mounted >>>> into the container to be able to perform the build. >>>> >>>> Since there is no generic way to find out all the directories that are >>>> needed, we need a manual mechanism. We choose the environment variable >>>> EXTRA_MOUNTPOINTS which contains a space-separated list of directories >>>> (or files) to mount. >>>> >>>> We choose an environment variable to avoid having to parse command-line >>>> arguments to docker-run. >>>> >>>> Update the terse documentation in utils/readme.txt with this >>>> information. >>>> >>>> Signed-off-by: Arnout Vandecappelle <arnout@mind.be> >>>> --- >>>> utils/docker-run | 5 +++++ >>>> utils/readme.txt | 5 ++++- >>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/utils/docker-run b/utils/docker-run >>>> index 1adb02d74e..3bb7b6a41b 100755 >>>> --- a/utils/docker-run >>>> +++ b/utils/docker-run >>>> @@ -90,6 +90,11 @@ if [ "${BR2_DL_DIR}" ]; then >>>> docker_opts+=( --env BR2_DL_DIR ) >>>> fi >>>> >>>> +if [ -n "${EXTRA_MOUNTPOINTS:-}" ]; then >>>> + read -r -a extra_mountpoints <<<"${EXTRA_MOUNTPOINTS}" >>> >>> I think it would be good to use `IFS=:` here and split EXTRA_MOUNTPOINTS >>> on ':'. That would make sure we don't split on other unexpected >>> characters like spaces, tabs, and newlines. And it would be consistent >>> with how BR2_EXTERNAL works. >> >> Well, there's a bigger chance that a file/directory contains a colon than that >> it contains a newline or tab, so : is not _that_ much better. Between tab and >> space I'd say they are about equally likely. >> >> However, a path with a space will simply not be supported by Buildroot. So if >> that extra mountpoint has a space and is used for a BR2_EXTERNAL, for a local >> tarball, for an override srcdir, for an output directory, for a host directory, >> for a download directory, or for some more things I'm forgetting, it simply will >> not work. >> >> On the other hand, a path that has a colon in it _can_ be used for all of >> these things, except for a BR2_EXTERNAL. >> >> So I'm no fan of using a colon. > > Yes that's a good point, it would be preferable to allow any characters > in a mount path, and a colon is probably more common then whitespace. Wouldn't it make sense to omit the "-r" option from read then, so people can escape the split character (whichever it ends up being)? The only disadvantage I see is that they'd also need potentially somewhat messy escaping for literal backslashes, but those seem very unlikely in paths. > One thing I also thought of, a colon would be consistent not only with > $BR2_EXTERNAL, but many common env variables use colons to seperate > paths. E.g. $PATH, $LD_LIBRARY_PATH, $MANPATH, etc. I'm assuming that > was why $BR2_EXTERNAL chose to use colons as well. That's why I'd prefer colons. On the other hand a bunch of BR2_* config variables use space separated lists, so BR2_EXTERNAL is kind of the odd one out from that point of view. Not sure if trying to unify the style in either direction is worth the trouble it'd cause, though. Best regards, Fiona > But this is a policy decision, so whichever one is used the code itself > look good. > > Reviewed-by: Brandon Maier <brandon.maier@collins.com> > > Thanks, > Brandon > >> >> Yann typically has a useful opinion about this kind of thing, so I'm putting >> him in Cc. >> >> Regards, >> Arnout >> >> >>> >>> Thanks, >>> Brandon >>> >>>> + mountpoints+=( "${extra_mountpoints[@]}" ) >>>> +fi >>>> + >>>> # shellcheck disable=SC2013 # can't use while-read because of the assignment >>>> for dir in $(printf '%s\n' "${mountpoints[@]}" |LC_ALL=C sort -u); do >>>> docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" ) >>>> diff --git a/utils/readme.txt b/utils/readme.txt >>>> index 6488d13c75..0cc665e478 100644 >>>> --- a/utils/readme.txt >>>> +++ b/utils/readme.txt >>>> @@ -21,7 +21,10 @@ check-package >>>> docker-run >>>> a script that runs a command (like make check-package) inside the >>>> buildroot CI docker container; pass no command to get an interactive >>>> - shell. >>>> + shell. If additional directories need to be accessible inside the >>>> + container, specify them with the environment variable EXTRA_MOUNTPOINTS. >>>> + The buildroot directory, the current directory, and the download >>>> + directory are automatically propagated. >>>> >>>> genrandconfig >>>> a script that generates a random configuration, used by the autobuilders >> _______________________________________________ >> buildroot mailing list >> buildroot@buildroot.org >> https://lists.buildroot.org/mailman/listinfo/buildroot
diff --git a/utils/docker-run b/utils/docker-run index 1adb02d74e..3bb7b6a41b 100755 --- a/utils/docker-run +++ b/utils/docker-run @@ -90,6 +90,11 @@ if [ "${BR2_DL_DIR}" ]; then docker_opts+=( --env BR2_DL_DIR ) fi +if [ -n "${EXTRA_MOUNTPOINTS:-}" ]; then + read -r -a extra_mountpoints <<<"${EXTRA_MOUNTPOINTS}" + mountpoints+=( "${extra_mountpoints[@]}" ) +fi + # shellcheck disable=SC2013 # can't use while-read because of the assignment for dir in $(printf '%s\n' "${mountpoints[@]}" |LC_ALL=C sort -u); do docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" ) diff --git a/utils/readme.txt b/utils/readme.txt index 6488d13c75..0cc665e478 100644 --- a/utils/readme.txt +++ b/utils/readme.txt @@ -21,7 +21,10 @@ check-package docker-run a script that runs a command (like make check-package) inside the buildroot CI docker container; pass no command to get an interactive - shell. + shell. If additional directories need to be accessible inside the + container, specify them with the environment variable EXTRA_MOUNTPOINTS. + The buildroot directory, the current directory, and the download + directory are automatically propagated. genrandconfig a script that generates a random configuration, used by the autobuilders
Sometimes, the build needs access to directories which are outside of the current directory, e.g. for pre-downloaded toolchains, local kernel sources, OVERRIDE_SRCDIR, BR2_EXTERNALs, ... We need these to be mounted into the container to be able to perform the build. Since there is no generic way to find out all the directories that are needed, we need a manual mechanism. We choose the environment variable EXTRA_MOUNTPOINTS which contains a space-separated list of directories (or files) to mount. We choose an environment variable to avoid having to parse command-line arguments to docker-run. Update the terse documentation in utils/readme.txt with this information. Signed-off-by: Arnout Vandecappelle <arnout@mind.be> --- utils/docker-run | 5 +++++ utils/readme.txt | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-)