[Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Arnout Vandecappelle via buildroot]

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}"
+    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
-- 
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 2/3] utils/docker-run: also mount externals

[Arnout Vandecappelle via buildroot]

When building in docker, we may want to include BR2_EXTERNALs that are
not in the current directory. These need to be mounted into the
container as well.

There is unfortunately no easy, generic way to find the list of
externals, except by running `make printvars`, but we'd need to run that
inside the container with all the command-line overrides set in order to
be fully correct. That is pretty hopeless, so as an approximation we
look for `.br2-external.mk` in the current directory.

Clearly this is quite limited because it doesn't support O= or
BR2_EXTERNAL= passed on the command line. This also means that the first
time, it doesn't work at all, because .br2-external.mk doesn't exist
yet. The EXTRA_MOUNTPOINTS feature can be used for that.

Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
---
Since this feature is not bulletproof, and EXTRA_MOUNTPOINTS already
covers the use case, it's perhaps not necessary to include this patch.
---
 utils/docker-run | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/utils/docker-run b/utils/docker-run
index 3bb7b6a41b..76fb81b074 100755
--- a/utils/docker-run
+++ b/utils/docker-run
@@ -29,6 +29,19 @@ declare -a mountpoints=(
     "$(pwd)"
 )
 
+# If any externals are defined, mount them as well. We assume that the current
+# directory is OUTPUT_DIR - ideally we'd parse O= and BR2_EXTERNAL= from the
+# command line but that's not really feasible in practice.
+if [ -e '.br2-external.mk' ]; then
+    mapfile -t mountpoints_external < <(
+        # We obviously want to quote the make syntax here
+        # shellcheck disable=SC2016
+        make -f .br2-external.mk -E \
+            'default:; @:$(foreach external,$(BR2_EXTERNAL_DIRS),$(info $(external)))'
+    )
+    mountpoints+=("${mountpoints_external[@]}")
+fi
+
 # We use the PODMAN_USERNS environment variable rather than using the
 # --userns command line argument because Fedora system may have the
 # podman-docker package installed, providing the "docker"
-- 
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 3/3] utils/brmake: allow override of MAKE

[Arnout Vandecappelle via buildroot]

Allow to override the 'make' program to use by setting the environment
variable MAKE (which is the standard environment variable used by make
itself).

This makes it possible to use an alternative make binary, or to provide
a wrapper around make. For example, to run the build itself under
docker, you can use:

    MAKE='utils/docker-run make' utils/brmake ...

(Running brmake itself under docker isn't possible because our image
doesn't have unbuffer.)

Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
---
 utils/brmake | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/utils/brmake b/utils/brmake
index 70dfb6cdc8..06977cc18d 100755
--- a/utils/brmake
+++ b/utils/brmake
@@ -2,6 +2,8 @@
 # (C) 2016, "Yann E. MORIN" <yann.morin.1998@free.fr>
 # License: WTFPL, https://spdx.org/licenses/WTFPL.html
 
+MAKE="${MAKE:-make}"
+
 main() {
     local ret start d h m mf
 
@@ -12,7 +14,9 @@ main() {
 
     start=${SECONDS}
 
-    ( exec 2>&1; unbuffer make "${@}"; ) \
+    # We want word splitting of ${MAKE}
+    # shellcheck disable=SC2086
+    ( exec 2>&1; unbuffer ${MAKE} "${@}"; ) \
     > >( while read -r line; do
              printf "%(%Y-%m-%dT%H:%M:%S)T %s\n" -1 "${line}"
          done \
-- 
2.45.2

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Brandon Maier via buildroot]

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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Arnout Vandecappelle via buildroot]

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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 2/3] utils/docker-run: also mount externals

[Brandon Maier via buildroot]

Hi Arnout,

On Sat Jul 13, 2024 at 2:43 PM UTC, Arnout Vandecappelle via buildroot wrote:
> When building in docker, we may want to include BR2_EXTERNALs that are
> not in the current directory. These need to be mounted into the
> container as well.
>
> There is unfortunately no easy, generic way to find the list of
> externals, except by running `make printvars`, but we'd need to run that
> inside the container with all the command-line overrides set in order to
> be fully correct. That is pretty hopeless, so as an approximation we
> look for `.br2-external.mk` in the current directory.

I have a maybe bad idea. But Buildroot does have a feature where it will
call `make` inside of `make` to set umask and other args, the '_all'
target in '$(TOPDIR)/Makefile'. Could we support all of these features
like this?

    ifeq ($(BR2_ENABLE_DOCKER),y)
    ifeq ($(BR2_SOME_VARIABLE_SET_INSIDE_DOCKER),)
    _all:
        $(TOPDIR)/utils/docker-run umask $(REQ_UMASK) && $(MAKE) ...
    else
    _all:
        ...
    endif

Buildroot has access to the actual BR2_EXTERNAL, OUTPUT_DIR, etc. So it
would be able to call docker-run with the correct arguments. And then it
would work when calling make from $(TOPDIR), or from inside custom
scripts like `brmake`.

Thanks,
Brandon

>
> Clearly this is quite limited because it doesn't support O= or
> BR2_EXTERNAL= passed on the command line. This also means that the first
> time, it doesn't work at all, because .br2-external.mk doesn't exist
> yet. The EXTRA_MOUNTPOINTS feature can be used for that.
>
> Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
> ---
> Since this feature is not bulletproof, and EXTRA_MOUNTPOINTS already
> covers the use case, it's perhaps not necessary to include this patch.
> ---
>  utils/docker-run | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/utils/docker-run b/utils/docker-run
> index 3bb7b6a41b..76fb81b074 100755
> --- a/utils/docker-run
> +++ b/utils/docker-run
> @@ -29,6 +29,19 @@ declare -a mountpoints=(
>      "$(pwd)"
>  )
>
> +# If any externals are defined, mount them as well. We assume that the current
> +# directory is OUTPUT_DIR - ideally we'd parse O= and BR2_EXTERNAL= from the
> +# command line but that's not really feasible in practice.
> +if [ -e '.br2-external.mk' ]; then
> +    mapfile -t mountpoints_external < <(
> +        # We obviously want to quote the make syntax here
> +        # shellcheck disable=SC2016
> +        make -f .br2-external.mk -E \
> +            'default:; @:$(foreach external,$(BR2_EXTERNAL_DIRS),$(info $(external)))'
> +    )
> +    mountpoints+=("${mountpoints_external[@]}")
> +fi
> +
>  # We use the PODMAN_USERNS environment variable rather than using the
>  # --userns command line argument because Fedora system may have the
>  # podman-docker package installed, providing the "docker"
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Brandon Maier via buildroot]

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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/3] utils/docker-run: allow to specify extra mount points to propagate

[Fiona Klute via 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

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot