[Buildroot] [PATCH v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Buildroot] [PATCH v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

The BR2_EXTERNAL environment variable is passed into the container,
and each path listed in it mounted. This allows using external trees
when running a build using utils/docker-run. Testing the existence of
the variable instead of a non-empty value allows passing an empty
BR2_EXTERNAL variable to disable currently set external trees.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
Changes v2 -> v3:
* Use read to make the loop that creates mount options more robust,
  removing pre-exisiting shellcheck override. Suggested by Brandon
  Maier.

Changes v1 -> v2:
* Correctly handle spaces in external tree paths

 utils/docker-run | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/utils/docker-run b/utils/docker-run
index 1adb02d74e..dbb9c45fdd 100755
--- a/utils/docker-run
+++ b/utils/docker-run
@@ -90,10 +90,17 @@ if [ "${BR2_DL_DIR}" ]; then
     docker_opts+=( --env BR2_DL_DIR )
 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
+if [ -v BR2_EXTERNAL ]; then
+    IFS=":" read -r -a br2_ext_arr <<< "${BR2_EXTERNAL}"
+    for br2_ext in "${br2_ext_arr[@]}"; do
+        mountpoints+=( "${br2_ext}" )
+    done
+    docker_opts+=( --env BR2_EXTERNAL )
+fi
+
+while IFS=$'\n' read -r dir; do
     docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" )
-done
+done < <(printf '%s\n' "${mountpoints[@]}" | LC_ALL=C sort -u)

 if tty -s; then
     docker_opts+=( -t )
--
2.45.2

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

[Buildroot] [PATCH v3 2/2] support/scripts/br2-external: allow spaces in dirname

[Fiona Klute via buildroot]

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

"make list-defconfigs" and "make nconfig" work in the container used
by utils/docker-run with external trees with spaces in the directory
name. Other build steps may or may not work.

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
This is mostly a proof-of-concept to show the previous patch works,
there might still be things that break in other parts of the build. I
have not checked if "printf '%q'" works in Bash 3.1 (like a comment
claims the script needs to support, not sure if still current).

Changes v1 -> v2:
* Added this patch

 support/scripts/br2-external | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/support/scripts/br2-external b/support/scripts/br2-external
index 8aea479d20..b8b6179aa8 100755
--- a/support/scripts/br2-external
+++ b/support/scripts/br2-external
@@ -34,7 +34,8 @@ main() {
     trap "error 'unexpected error while generating ${ofile}\n'" ERR

     mkdir -p "${outputdir}"
-    do_validate "${outputdir}" ${@//:/ }
+    IFS=":" read -r -a br2_extdirs <<< "${@}"
+    do_validate "${outputdir}" "${br2_extdirs[@]}"
     do_mk "${outputdir}"
     do_kconfig "${outputdir}"
 }
@@ -144,9 +145,9 @@ do_mk() {
             eval br2_ver="\"\${BR2_EXT_VERS_${br2_name}}\""
             printf '\n'
             printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}"
-            printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
-            printf 'BR2_EXTERNAL_MKS += %s/external.mk\n' "${br2_ext}"
-            printf 'export BR2_EXTERNAL_%s_PATH = %s\n' "${br2_name}" "${br2_ext}"
+            printf 'BR2_EXTERNAL_DIRS += %q\n' "${br2_ext}"
+            printf 'BR2_EXTERNAL_MKS += %q/external.mk\n' "${br2_ext}"
+            printf 'export BR2_EXTERNAL_%s_PATH = %q\n' "${br2_name}" "${br2_ext}"
             printf 'export BR2_EXTERNAL_%s_DESC = %s\n' "${br2_name}" "${br2_desc}"
             printf 'export BR2_EXTERNAL_%s_VERSION = %s\n' "${br2_name}" "${br2_ver}"
         done
--
2.45.2

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

Re: [Buildroot] [PATCH v3 2/2] support/scripts/br2-external: allow spaces in dirname

[Brandon Maier via buildroot]

On Sat Jul 13, 2024 at 7:22 PM UTC, Fiona Klute via buildroot wrote:
> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>
> "make list-defconfigs" and "make nconfig" work in the container used
> by utils/docker-run with external trees with spaces in the directory
> name. Other build steps may or may not work.
>
> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>

Reviewed-by: Brandon Maier <brandon.maier@collins.com>

> ---
> This is mostly a proof-of-concept to show the previous patch works,
> there might still be things that break in other parts of the build. I
> have not checked if "printf '%q'" works in Bash 3.1 (like a comment
> claims the script needs to support, not sure if still current).
>
> Changes v1 -> v2:
> * Added this patch
>
>  support/scripts/br2-external | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/support/scripts/br2-external b/support/scripts/br2-external
> index 8aea479d20..b8b6179aa8 100755
> --- a/support/scripts/br2-external
> +++ b/support/scripts/br2-external
> @@ -34,7 +34,8 @@ main() {
>      trap "error 'unexpected error while generating ${ofile}\n'" ERR
>
>      mkdir -p "${outputdir}"
> -    do_validate "${outputdir}" ${@//:/ }
> +    IFS=":" read -r -a br2_extdirs <<< "${@}"
> +    do_validate "${outputdir}" "${br2_extdirs[@]}"
>      do_mk "${outputdir}"
>      do_kconfig "${outputdir}"
>  }
> @@ -144,9 +145,9 @@ do_mk() {
>              eval br2_ver="\"\${BR2_EXT_VERS_${br2_name}}\""
>              printf '\n'
>              printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}"
> -            printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
> -            printf 'BR2_EXTERNAL_MKS += %s/external.mk\n' "${br2_ext}"
> -            printf 'export BR2_EXTERNAL_%s_PATH = %s\n' "${br2_name}" "${br2_ext}"
> +            printf 'BR2_EXTERNAL_DIRS += %q\n' "${br2_ext}"
> +            printf 'BR2_EXTERNAL_MKS += %q/external.mk\n' "${br2_ext}"
> +            printf 'export BR2_EXTERNAL_%s_PATH = %q\n' "${br2_name}" "${br2_ext}"
>              printf 'export BR2_EXTERNAL_%s_DESC = %s\n' "${br2_name}" "${br2_desc}"
>              printf 'export BR2_EXTERNAL_%s_VERSION = %s\n' "${br2_name}" "${br2_ver}"
>          done
> --
> 2.45.2
>
> _______________________________________________
> 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 v3 2/2] support/scripts/br2-external: allow spaces in dirname

[Yann E. MORIN]

Fiona, All,

On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
> "make list-defconfigs" and "make nconfig" work in the container used
> by utils/docker-run with external trees with spaces in the directory
> name. Other build steps may or may not work.

As you state yourself, "Other build steps may or may not work". And
indeed, that's probably not going to work, as BR2_EXTERNAL is expected
to be a space-separated list, that will be split on spaces from Makefile
context. So, no amount of quoting will make br2-external trees with
spaces actually work (AFAICS).

Regards,
Yann E. MORIN.

> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
> ---
> This is mostly a proof-of-concept to show the previous patch works,
> there might still be things that break in other parts of the build. I
> have not checked if "printf '%q'" works in Bash 3.1 (like a comment
> claims the script needs to support, not sure if still current).
> 
> Changes v1 -> v2:
> * Added this patch
> 
>  support/scripts/br2-external | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/support/scripts/br2-external b/support/scripts/br2-external
> index 8aea479d20..b8b6179aa8 100755
> --- a/support/scripts/br2-external
> +++ b/support/scripts/br2-external
> @@ -34,7 +34,8 @@ main() {
>      trap "error 'unexpected error while generating ${ofile}\n'" ERR
> 
>      mkdir -p "${outputdir}"
> -    do_validate "${outputdir}" ${@//:/ }
> +    IFS=":" read -r -a br2_extdirs <<< "${@}"
> +    do_validate "${outputdir}" "${br2_extdirs[@]}"
>      do_mk "${outputdir}"
>      do_kconfig "${outputdir}"
>  }
> @@ -144,9 +145,9 @@ do_mk() {
>              eval br2_ver="\"\${BR2_EXT_VERS_${br2_name}}\""
>              printf '\n'
>              printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}"
> -            printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
> -            printf 'BR2_EXTERNAL_MKS += %s/external.mk\n' "${br2_ext}"
> -            printf 'export BR2_EXTERNAL_%s_PATH = %s\n' "${br2_name}" "${br2_ext}"
> +            printf 'BR2_EXTERNAL_DIRS += %q\n' "${br2_ext}"
> +            printf 'BR2_EXTERNAL_MKS += %q/external.mk\n' "${br2_ext}"
> +            printf 'export BR2_EXTERNAL_%s_PATH = %q\n' "${br2_name}" "${br2_ext}"
>              printf 'export BR2_EXTERNAL_%s_DESC = %s\n' "${br2_name}" "${br2_desc}"
>              printf 'export BR2_EXTERNAL_%s_VERSION = %s\n' "${br2_name}" "${br2_ver}"
>          done
> --
> 2.45.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3 2/2] support/scripts/br2-external: allow spaces in dirname

[Fiona Klute via buildroot]

Am 30.08.24 um 23:38 schrieb Yann E. MORIN:
> Fiona, All,
>
> On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>> "make list-defconfigs" and "make nconfig" work in the container used
>> by utils/docker-run with external trees with spaces in the directory
>> name. Other build steps may or may not work.
>
> As you state yourself, "Other build steps may or may not work". And
> indeed, that's probably not going to work, as BR2_EXTERNAL is expected
> to be a space-separated list, that will be split on spaces from Makefile
> context. So, no amount of quoting will make br2-external trees with
> spaces actually work (AFAICS).

BR2_EXTERNAL is colon separated, the current code simply replaces colons
with spaces and then applies word splitting:

do_validate "${outputdir}" ${@//:/ }

If writing the paths to the makefiles quoted isn't safe (it does work
e.g. with "make list-defconfigs", but I don't know what would happen
later in the build) then I think do_validate_one should check for spaces
in the paths and error out if there are any. Sure, the "does the path
exist" check is likely to fail anyway on a path split in the middle, but
the error message would equally likely be a bit confusing.

Best regards,
Fiona

> Regards,
> Yann E. MORIN.
>
>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>> ---
>> This is mostly a proof-of-concept to show the previous patch works,
>> there might still be things that break in other parts of the build. I
>> have not checked if "printf '%q'" works in Bash 3.1 (like a comment
>> claims the script needs to support, not sure if still current).
>>
>> Changes v1 -> v2:
>> * Added this patch
>>
>>   support/scripts/br2-external | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/support/scripts/br2-external b/support/scripts/br2-external
>> index 8aea479d20..b8b6179aa8 100755
>> --- a/support/scripts/br2-external
>> +++ b/support/scripts/br2-external
>> @@ -34,7 +34,8 @@ main() {
>>       trap "error 'unexpected error while generating ${ofile}\n'" ERR
>>
>>       mkdir -p "${outputdir}"
>> -    do_validate "${outputdir}" ${@//:/ }
>> +    IFS=":" read -r -a br2_extdirs <<< "${@}"
>> +    do_validate "${outputdir}" "${br2_extdirs[@]}"
>>       do_mk "${outputdir}"
>>       do_kconfig "${outputdir}"
>>   }
>> @@ -144,9 +145,9 @@ do_mk() {
>>               eval br2_ver="\"\${BR2_EXT_VERS_${br2_name}}\""
>>               printf '\n'
>>               printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}"
>> -            printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
>> -            printf 'BR2_EXTERNAL_MKS += %s/external.mk\n' "${br2_ext}"
>> -            printf 'export BR2_EXTERNAL_%s_PATH = %s\n' "${br2_name}" "${br2_ext}"
>> +            printf 'BR2_EXTERNAL_DIRS += %q\n' "${br2_ext}"
>> +            printf 'BR2_EXTERNAL_MKS += %q/external.mk\n' "${br2_ext}"
>> +            printf 'export BR2_EXTERNAL_%s_PATH = %q\n' "${br2_name}" "${br2_ext}"
>>               printf 'export BR2_EXTERNAL_%s_DESC = %s\n' "${br2_name}" "${br2_desc}"
>>               printf 'export BR2_EXTERNAL_%s_VERSION = %s\n' "${br2_name}" "${br2_ver}"
>>           done
>> --
>> 2.45.2
>>
>> _______________________________________________
>> 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 v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Brandon Maier via buildroot]

Hi Fiona,

On Sat Jul 13, 2024 at 7:22 PM UTC, Fiona Klute via buildroot wrote:
> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>
> The BR2_EXTERNAL environment variable is passed into the container,
> and each path listed in it mounted. This allows using external trees
> when running a build using utils/docker-run. Testing the existence of
> the variable instead of a non-empty value allows passing an empty
> BR2_EXTERNAL variable to disable currently set external trees.
>
> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>

Reviewed-by: Brandon Maier <brandon.maier@collins.com>

> ---
> Changes v2 -> v3:
> * Use read to make the loop that creates mount options more robust,
>   removing pre-exisiting shellcheck override. Suggested by Brandon
>   Maier.
>
> Changes v1 -> v2:
> * Correctly handle spaces in external tree paths
>
>  utils/docker-run | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/utils/docker-run b/utils/docker-run
> index 1adb02d74e..dbb9c45fdd 100755
> --- a/utils/docker-run
> +++ b/utils/docker-run
> @@ -90,10 +90,17 @@ if [ "${BR2_DL_DIR}" ]; then
>      docker_opts+=( --env BR2_DL_DIR )
>  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
> +if [ -v BR2_EXTERNAL ]; then
> +    IFS=":" read -r -a br2_ext_arr <<< "${BR2_EXTERNAL}"
> +    for br2_ext in "${br2_ext_arr[@]}"; do
> +        mountpoints+=( "${br2_ext}" )
> +    done
> +    docker_opts+=( --env BR2_EXTERNAL )
> +fi
> +
> +while IFS=$'\n' read -r dir; do
>      docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" )
> -done
> +done < <(printf '%s\n' "${mountpoints[@]}" | LC_ALL=C sort -u)
>
>  if tty -s; then
>      docker_opts+=( -t )
> --
> 2.45.2
>
> _______________________________________________
> 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 v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Yann E. MORIN]

Fiona, All,

On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
> The BR2_EXTERNAL environment variable is passed into the container,
> and each path listed in it mounted. This allows using external trees
> when running a build using utils/docker-run. Testing the existence of
> the variable instead of a non-empty value allows passing an empty
> BR2_EXTERNAL variable to disable currently set external trees.
> 
> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
> ---
> Changes v2 -> v3:
> * Use read to make the loop that creates mount options more robust,
>   removing pre-exisiting shellcheck override. Suggested by Brandon
>   Maier.
> 
> Changes v1 -> v2:
> * Correctly handle spaces in external tree paths
> 
>  utils/docker-run | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/docker-run b/utils/docker-run
> index 1adb02d74e..dbb9c45fdd 100755
> --- a/utils/docker-run
> +++ b/utils/docker-run
> @@ -90,10 +90,17 @@ if [ "${BR2_DL_DIR}" ]; then
>      docker_opts+=( --env BR2_DL_DIR )
>  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
> +if [ -v BR2_EXTERNAL ]; then
> +    IFS=":" read -r -a br2_ext_arr <<< "${BR2_EXTERNAL}"
> +    for br2_ext in "${br2_ext_arr[@]}"; do
> +        mountpoints+=( "${br2_ext}" )
> +    done
> +    docker_opts+=( --env BR2_EXTERNAL )
> +fi
> +
> +while IFS=$'\n' read -r dir; do
>      docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" )
> -done
> +done < <(printf '%s\n' "${mountpoints[@]}" | LC_ALL=C sort -u)

This commit mixes multiple changes:
  - using process substitution with a while-loop, instead of the
    command substitution and a for-loop
  - passing br2-external and mounting trees in the container
  - handling br2-external trees with spaces in them

I see you have a second patch with support for br2-external trees with
spaces in their paths. If at all, it should come before this one.

However, I don't think we should support paths with spaces. We currently
do not, and the rest of the code base (as your second patch states) may
or may not work (hint: it does not, it's broken in a lot of places, and
the buildsystems of many packages do not support spaces in path either).

Also, BR2_EXTERNAL must be a space-separated list, as it needs to be
split on space from Makefile context, so no br2-external tree with
spaces in paths can even work... (unless I missed something...)

So, I'm OK with passing br2-external to the container, but not with the
rest of the changes in this file. I.e. the change whould be just that:

    docker_opts+=( --env BR2_EXTERNAL )
    for dir in ${BR2_EXTERNAL}; do
        mountpoints+=( "${br2_ext}" )
    done

Regards,
Yann E. MORIN.

>  if tty -s; then
>      docker_opts+=( -t )
> --
> 2.45.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Fiona Klute via buildroot]

Hi Yann, all!

Am 30.08.24 um 23:35 schrieb Yann E. MORIN:
> Fiona, All,
>
> On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>> The BR2_EXTERNAL environment variable is passed into the container,
>> and each path listed in it mounted. This allows using external trees
>> when running a build using utils/docker-run. Testing the existence of
>> the variable instead of a non-empty value allows passing an empty
>> BR2_EXTERNAL variable to disable currently set external trees.
>>
>> Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
>> ---
>> Changes v2 -> v3:
>> * Use read to make the loop that creates mount options more robust,
>>    removing pre-exisiting shellcheck override. Suggested by Brandon
>>    Maier.
>>
>> Changes v1 -> v2:
>> * Correctly handle spaces in external tree paths
>>
>>   utils/docker-run | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/docker-run b/utils/docker-run
>> index 1adb02d74e..dbb9c45fdd 100755
>> --- a/utils/docker-run
>> +++ b/utils/docker-run
>> @@ -90,10 +90,17 @@ if [ "${BR2_DL_DIR}" ]; then
>>       docker_opts+=( --env BR2_DL_DIR )
>>   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
>> +if [ -v BR2_EXTERNAL ]; then
>> +    IFS=":" read -r -a br2_ext_arr <<< "${BR2_EXTERNAL}"
>> +    for br2_ext in "${br2_ext_arr[@]}"; do
>> +        mountpoints+=( "${br2_ext}" )
>> +    done
>> +    docker_opts+=( --env BR2_EXTERNAL )
>> +fi
>> +
>> +while IFS=$'\n' read -r dir; do
>>       docker_opts+=( --mount "type=bind,src=${dir},dst=${dir}" )
>> -done
>> +done < <(printf '%s\n' "${mountpoints[@]}" | LC_ALL=C sort -u)
>
> This commit mixes multiple changes:
>    - using process substitution with a while-loop, instead of the
>      command substitution and a for-loop
>    - passing br2-external and mounting trees in the container
>    - handling br2-external trees with spaces in them
>
> I see you have a second patch with support for br2-external trees with
> spaces in their paths. If at all, it should come before this one.
>
> However, I don't think we should support paths with spaces. We currently
> do not, and the rest of the code base (as your second patch states) may
> or may not work (hint: it does not, it's broken in a lot of places, and
> the buildsystems of many packages do not support spaces in path either).
>
> Also, BR2_EXTERNAL must be a space-separated list, as it needs to be
> split on space from Makefile context, so no br2-external tree with
> spaces in paths can even work... (unless I missed something...)

According to the manual multiple paths in BR2_EXTERNAL must be separated
by colons, not spaces, example copied verbatim from
docs/manual/customize-outside-br.adoc:

buildroot/ $ make BR2_EXTERNAL=/path/to/foo:/where/we/have/bar menuconfig

support/scripts/br2-external splits BR2_EXTERNAL by colon, or rather
replaces colons with spaces in the variable value and then uses the
result unquoted to create a list of parameters. That means
utils/docker-run must split by colon, too, no matter if we want to make
the handling of the individual paths after splitting space-safe or not.

> So, I'm OK with passing br2-external to the container, but not with the
> rest of the changes in this file. I.e. the change whould be just that:
>
>      docker_opts+=( --env BR2_EXTERNAL )
>      for dir in ${BR2_EXTERNAL}; do
>          mountpoints+=( "${br2_ext}" )
>      done
I can drop the change to handle spaces in paths in the loop that
constructs docker_opts elements from mountpoints, but the read to split
$BR2_EXTERNAL by ":" needs to stay or handling multiple external trees
won't work.

That said, I don't really see a reason *not* to make utils/docker-run
handle paths with spaces correctly when it doesn't make the script
significantly more complex. That's at least one less place where they
could cause unpleasant surprises. The alternative would be to raise an
error directly, but I think the right place for that would be
support/scripts/br2-external (e.g. by extending the second patch in this
series to check the values after splitting instead of quoting possible
spaces).

Best regards,
Fiona

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

Re: [Buildroot] [PATCH v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Yann E. MORIN]

Fiona, All,

On 2024-08-31 18:57 +0200, Fiona Klute spake thusly:
> Am 30.08.24 um 23:35 schrieb Yann E. MORIN:
> > On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
> > > From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
> > > The BR2_EXTERNAL environment variable is passed into the container,
> > > and each path listed in it mounted. This allows using external trees
> > > when running a build using utils/docker-run. Testing the existence of
> > > the variable instead of a non-empty value allows passing an empty
> > > BR2_EXTERNAL variable to disable currently set external trees.
[--SNIP--]
> > This commit mixes multiple changes:
> >    - using process substitution with a while-loop, instead of the
> >      command substitution and a for-loop
> >    - passing br2-external and mounting trees in the container
> >    - handling br2-external trees with spaces in them
> > 
> > I see you have a second patch with support for br2-external trees with
> > spaces in their paths. If at all, it should come before this one.
> > 
> > However, I don't think we should support paths with spaces. We currently
> > do not, and the rest of the code base (as your second patch states) may
> > or may not work (hint: it does not, it's broken in a lot of places, and
> > the buildsystems of many packages do not support spaces in path either).
> > 
> > Also, BR2_EXTERNAL must be a space-separated list, as it needs to be
> > split on space from Makefile context, so no br2-external tree with
> > spaces in paths can even work... (unless I missed something...)
> 
> According to the manual multiple paths in BR2_EXTERNAL must be separated
> by colons, not spaces, example copied verbatim from
> docs/manual/customize-outside-br.adoc:
> 
> buildroot/ $ make BR2_EXTERNAL=/path/to/foo:/where/we/have/bar menuconfig

Supporting both colon- and space-separated lists was the worst idea I
had when I implemented and coded support for mutliple br2-externals.

In fact, the manual has an example with a colon, but nowhere does it
explicitly states that it can be either.

> support/scripts/br2-external splits BR2_EXTERNAL by colon, or rather
> replaces colons with spaces in the variable value and then uses the
> result unquoted to create a list of parameters.

Note that the code (which you quoted in your reply to patch 2) is:

    do_validate "${outputdir}" ${@//:/ }

So, it is going to split the words on spaces. E.g. passing:
    BR2_EXTERNAL=/path/to some/dir:/other/path

will result in this call:

    do_validate "${outputdir}" /path/to some/dir /other/path

which will be three arguments. And that would fail:

    $ BR2_EXTERNAL="/home/ymorin/dev/buildroot/misc/br2-external-tests/br2-external-1:/home/ymorin/dev/buildroot/misc/br2-external-tests/br2 external" make defconfig
    Makefile:194: *** '/home/ymorin/dev/buildroot/misc/br2-external-tests/br2': no such file or directory.  Stop.
    make: *** [Makefile:23: _all] Error 2

(I need to polish and push my br2-external test trees to gitlab...)

Also, internally, BR2_EXTERNAL is saved back as a space-separated list:

    https://gitlab.com/buildroot.org/buildroot/-/blob/master/support/scripts/br2-external?ref_type=heads#L124

    printf 'BR2_EXTERNAL ?='
    for br2_name in "${BR2_EXT_NAMES[@]}"; do
        eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\""
        printf ' %s' "${br2_ext}"
    done
    printf '\n'

... so that calling 'make' again without specifyig BR2_EXTERNAL will
still use the BR2_EXTERNAL trees used at configure time.

So, definitely, br2-external trees with spaces in them is not supported
and is not working.

> That means
> utils/docker-run must split by colon, too, no matter if we want to make
> the handling of the individual paths after splitting space-safe or not.

Damn right you are, damit! ;-) So the code could just look like:

     docker_opts+=( --env BR2_EXTERNAL )
     for dir in ${BR2_EXTERNAL//:/ }; do
         mountpoints+=( "${br2_ext}" )
     done

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3 1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs

[Fiona Klute via buildroot]

Am 31.08.24 um 20:14 schrieb Yann E. MORIN:
> Fiona, All,
>
> On 2024-08-31 18:57 +0200, Fiona Klute spake thusly:
>> Am 30.08.24 um 23:35 schrieb Yann E. MORIN:
>>> On 2024-07-13 21:22 +0200, Fiona Klute via buildroot spake thusly:
>>>> From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
>>>> The BR2_EXTERNAL environment variable is passed into the container,
>>>> and each path listed in it mounted. This allows using external trees
>>>> when running a build using utils/docker-run. Testing the existence of
>>>> the variable instead of a non-empty value allows passing an empty
>>>> BR2_EXTERNAL variable to disable currently set external trees.
> [--SNIP--]
>>> This commit mixes multiple changes:
>>>     - using process substitution with a while-loop, instead of the
>>>       command substitution and a for-loop
>>>     - passing br2-external and mounting trees in the container
>>>     - handling br2-external trees with spaces in them
>>>
>>> I see you have a second patch with support for br2-external trees with
>>> spaces in their paths. If at all, it should come before this one.
>>>
>>> However, I don't think we should support paths with spaces. We currently
>>> do not, and the rest of the code base (as your second patch states) may
>>> or may not work (hint: it does not, it's broken in a lot of places, and
>>> the buildsystems of many packages do not support spaces in path either).
>>>
>>> Also, BR2_EXTERNAL must be a space-separated list, as it needs to be
>>> split on space from Makefile context, so no br2-external tree with
>>> spaces in paths can even work... (unless I missed something...)
>>
>> According to the manual multiple paths in BR2_EXTERNAL must be separated
>> by colons, not spaces, example copied verbatim from
>> docs/manual/customize-outside-br.adoc:
>>
>> buildroot/ $ make BR2_EXTERNAL=/path/to/foo:/where/we/have/bar menuconfig
>
> Supporting both colon- and space-separated lists was the worst idea I
> had when I implemented and coded support for mutliple br2-externals.
>
> In fact, the manual has an example with a colon, but nowhere does it
> explicitly states that it can be either.

Okay, in that case there really is no nice solution. :-(

I've just sent an updated patch based on your suggestion.

Best regards,
Fiona

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