support/br2-external: fix shellcheck and better document BR2_EXTERNAL (branch yem/misc)

The trap was initially introduced in c5fa9308ea4e (core/br2-external:
properly report unexpected errors), in 2017, to catch all unexpected errors, back when a single file was generated, and errors emitted to stderr.

Since commit d027cd75d098 (core: generate all br2-external files in one go), in 2019 the single output file 'ofile' is no longer created, as multiple output files were then introduced, while messages for *expected errors* were redirected to a Makefile variable assignment emitted on stdout, at which point the script just exits (in error); expected failures only occur in do_validate().

Unexpected errors can only occur on failure to create, or write to, output files, either '.br2-external.mk' in do_validate() or do_mk(), or any of the kconfig fragments in do_kconfig(). Cause for failure to create those can only be a no-space-left-on-device condition, as they are created in a directory that was just created by the script earlier in main(), and thus has the necessary mode; failure to create that directory is now caught explicitly.

A trap on ERR is not called when the shell exits explicitly with a call to 'exit', thus, only failures to create or write to output file would be caught. In that case, we are better off not trying to write to those files anyway: failure to create the file would already be reported by the shell on stderr, while disk-full would not allow to store the output anyway...

In any case, the script exits in error, which is going to be caught by the caller, which will terminate.

So, drop the trap altogether.

As a side effect, that squelches a shellcheck error.

Signed-off-by Yann E. MORIN <[email protected]>

---
Note: remaining shellcheck errors will be addressed separately in the following patch.

---
 support/scripts/br2-external | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

support/scripts/br2-external +0 -0

index 8aea479d20..6d4fc1a791 100755
@@ -30,10 +30,10 @@ main() {
         error "no output directory specified (-d)\n"
     fi
 

The trap was initially introduced in c5fa9308ea4e (core/br2-external:
properly report unexpected errors), in 2017, to catch all unexpected errors, back when a single file was generated, and errors emitted to stderr.

Since commit d027cd75d098 (core: generate all br2-external files in one go), in 2019 the single output file 'ofile' is no longer created, as multiple output files were then introduced, while messages for *expected errors* were redirected to a Makefile variable assignment emitted on stdout, at which point the script just exits (in error); expected failures only occur in do_validate().

Unexpected errors can only occur on failure to create, or write to, output files, either '.br2-external.mk' in do_validate() or do_mk(), or any of the kconfig fragments in do_kconfig(). Cause for failure to create those can only be a no-space-left-on-device condition, as they are created in a directory that was just created by the script earlier in main(), and thus has the necessary mode; failure to create that directory is now caught explicitly.

A trap on ERR is not called when the shell exits explicitly with a call to 'exit', thus, only failures to create or write to output file would be caught. In that case, we are better off not trying to write to those files anyway: failure to create the file would already be reported by the shell on stderr, while disk-full would not allow to store the output anyway...

In any case, the script exits in error, which is going to be caught by the caller, which will terminate.

So, drop the trap altogether.

As a side effect, that squelches a shellcheck error.

Signed-off-by Yann E. MORIN <[email protected]>

---
Note: remaining shellcheck errors will be addressed separately in the following patch.

---
 support/scripts/br2-external | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

support/scripts/br2-external +3 -3

index 8aea479d20..6d4fc1a791 100755
@@ -30,10 +30,10 @@ main() {
         error "no output directory specified (-d)\n"
     fi
 
-    # Trap any unexpected error to generate a meaningful error message
-    trap "error 'unexpected error while generating ${ofile}\n'" ERR
+    if ! mkdir -p "${outputdir}"; then
+        error "Cannot create output directory '%s'\n" "${outputdir}"
+    fi
 
-    mkdir -p "${outputdir}"
     do_validate "${outputdir}" ${@//:/ }
     do_mk "${outputdir}"
     do_kconfig "${outputdir}"

Signature · 6 lines

2.52.0

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

Boring changes: either do what shellcheck suggested, or comment why we don't want to fix the code.

Signed-off-by Yann E. MORIN <[email protected]>

---
 .checkpackageignore          |  1 -
 support/scripts/br2-external | 11 ++++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

.checkpackageignore +0 -1

index 16a0d5b511..67e531512e 100644
@@ -1085,7 +1085,6 @@ support/libtool/buildroot-libtool-v2.4.4.patch lib_patch.ApplyOrder lib_patch.Up
 support/libtool/buildroot-libtool-v2.4.patch lib_patch.ApplyOrder lib_patch.Sob lib_patch.Upstream
 support/misc/relocate-sdk.sh Shellcheck
 support/scripts/apply-patches.sh Shellcheck
-support/scripts/br2-external Shellcheck
 support/scripts/check-bin-arch Shellcheck
 support/scripts/check-host-rpath Shellcheck
 support/scripts/expunge-gconv-modules Shellcheck

support/scripts/br2-external +6 -5

index 6d4fc1a791..474feaded7 100755
@@ -34,6 +34,7 @@ main() {
         error "Cannot create output directory '%s'\n" "${outputdir}"
     fi
 
+    # shellcheck disable=SC2068  # We do want to split on spaces
     do_validate "${outputdir}" ${@//:/ }
     do_mk "${outputdir}"
     do_kconfig "${outputdir}"

@@ -71,7 +72,7 @@ do_validate_one() {
     if [ ! -d "${br2_ext}" ]; then
         error "'%s': no such file or directory\n" "${br2_ext}"
     fi
-    if [ ! -r "${br2_ext}" -o ! -x "${br2_ext}" ]; then
+    if [ ! -r "${br2_ext}" ] || [ ! -x "${br2_ext}" ]; then
         error "'%s': permission denied\n" "${br2_ext}"
     fi
     if [ ! -f "${br2_ext}/external.desc" ]; then

@@ -106,9 +107,9 @@ do_validate_one() {
     br2_ext="$( cd "${br2_ext}"; pwd )"
     br2_ver="$( support/scripts/setlocalversion "${br2_ext}" )"
     BR2_EXT_NAMES+=( "${br2_name}" )
-    eval BR2_EXT_PATHS_${br2_name}="\"\${br2_ext}\""
-    eval BR2_EXT_VERS_${br2_name}="\"\${br2_ver}\""
-    eval BR2_EXT_DESCS_${br2_name}="\"\${br2_desc:-\${br2_name}}\""
+    eval "BR2_EXT_PATHS_${br2_name}=\"\${br2_ext}\""
+    eval "BR2_EXT_VERS_${br2_name}=\"\${br2_ver}\""
+    eval "BR2_EXT_DESCS_${br2_name}=\"\${br2_desc:-\${br2_name}}\""
 }
 
 # Generate the .mk snippet that defines makefile variables

@@ -272,7 +273,7 @@ do_kconfig() {
     printf 'endmenu\n' >>"${outputdir}/.br2-external.in.menus"
 }
 
+# shellcheck disable=SC2059  # fmt *is* a format
 error() { local fmt="${1}"; shift; printf "BR2_EXTERNAL_ERROR = ${fmt}" "${@}"; exit 1; }
 
-my_name="${0##*/}"
 main "${@}"

Signature · 6 lines

2.52.0

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

Specifying a list of br2-external trees is poorly documented, and the only example uses a colon to separate the br2-external paths.

Adding the support for colon-separated list is the biggest mistake that was made when introducing support for multiple br2-external [0]. Indeed, both space and colon can be used to separate entries in the list, and it is also possible to mix the two. However, internally, the list is stored as a space-separated list, and all the code will split on spaces.

Besides, all other lists in Buildroot are a space-separated:

    BR2_ROOTFS_DEVICE_TABLE
    BR2_ROOTFS_STATIC_DEVICE_TABLE
    BR2_TARGET_TZ_ZONELIST
    BR2_ROOTFS_USERS_TABLES
    BR2_ROOTFS_OVERLAY
    BR2_ROOTFS_PRE_BUILD_SCRIPT
    BR2_ROOTFS_POST_BUILD_SCRIPT
    BR2_ROOTFS_POST_FAKEROOT_SCRIPT
    BR2_ROOTFS_POST_IMAGE_SCRIPT
    ...

So, using colons is odd.

The fact that BR2_EXTERNAL is pqssed on the command line rather than being a Kconfig item is not a reason enough to justify that it be colon-separated.

Change the documentation to only mention using a space-separated list.

Of course, for backward compatibility, we keep the code as-is to accept a colon-separated list, but we just do not advertise it.

Note that keeping the split on colons means that colons are not accepted in pathnames of br2-external trees; in practice, this is not a new restriction, or one that could lift as usign colons in Makefiles are problematic anyway.

[0] in 20cd49738781 core: add support for multiple br2-external trees

Reported-by Fiona Klute (WIWA) <[email protected]>

Reported-by Brandon Maier <[email protected]>

Signed-off-by Yann E. MORIN <[email protected]>

Cc Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
Note: support/scripts/br2-external has shellcheck issues; this commit does not try to address those on purpose, as it only adds a comment.

---
Changes v3 -> v4:
- add an extract of all other list variables from [1]
- sumarise and include Arnout's comments from [2]
- fix typo

[1] https://patchwork.ozlabs.org/project/buildroot/patch/[email protected]/#3375145
[2] https://patchwork.ozlabs.org/project/buildroot/patch/[email protected]/#3375416

Changes v2 -> v3:
- add comment in support/scripts/br2-external (Fiona)

Changes v1 -> v2:
- fix typoes

---
 docs/manual/customize-outside-br.adoc | 5 +++--
 support/scripts/br2-external          | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

docs/manual/customize-outside-br.adoc +3 -2

index 78065489d4..8a76f3a998 100644
@@ -58,10 +58,11 @@ We can switch to another br2-external tree at any time:
 buildroot/ $ make BR2_EXTERNAL=/where/we/have/bar xconfig
 ----
 
-We can also use multiple br2-external trees:
+We can also use multiple br2-external trees, by specifying a space-separated
+list of paths to use:
 
 ----
-buildroot/ $ make BR2_EXTERNAL=/path/to/foo:/where/we/have/bar menuconfig
+buildroot/ $ make BR2_EXTERNAL="/path/to/foo /where/we/have/bar" menuconfig
 ----
 
 Or disable the usage of any br2-external tree:

support/scripts/br2-external +3 -0

index 474feaded7..641f8e18ae 100755
@@ -34,6 +34,9 @@ main() {
         error "Cannot create output directory '%s'\n" "${outputdir}"
     fi
 
+    # Historically, BR2_EXTERNAL could also be colon-separated, so for
+    # backward compatibility, keep splitting on colons (in addition to
+    # spaces).
     # shellcheck disable=SC2068  # We do want to split on spaces
     do_validate "${outputdir}" ${@//:/ }
     do_mk "${outputdir}"

Signature · 6 lines

2.52.0

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot