| Message ID | 20240713192207.873065-1-fiona.klute@gmx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/2] utils/docker-run: mount and pass BR2_EXTERNAL dirs | expand |
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
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
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
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.
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
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 )