core/br2-external: restore compatibility with old distros
PATCH 1/1 core/br2-external: restore compatibility with old distros
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
We restore compatibility with those oldish distros using 'eval' to emulate associative arrays, as suggested by Arnout.
--- support/scripts/br2-external | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
support/scripts/br2-external +17 -17
@@ -1,10 +1,12 @@ #!/bin/bash set -e -# The names and locations of the br2-external trees, once validated. +# This script must be able to run with bash-3.1, so it can't use +# associative arrays. Instead, it emulates them using 'eval'. It +# can however use indexed arrays, supported since at least bash-3.0. + +# The names of the br2-external trees, once validated. declare -a BR2_EXT_NAMES -declare -A BR2_EXT_PATHS -declare -A BR2_EXT_DESCS # URL to manual for help in converting old br2-external trees. # Escape '#' so that make does not consider it a comment.
@@ -68,7 +70,7 @@ do_validate() { do_validate_one() { local br2_ext="${1}" - local br2_name br2_desc n + local br2_name br2_desc n d if [ ! -d "${br2_ext}" ]; then error "'%s': no such file or directory\n" "${br2_ext}"
@@ -91,9 +93,10 @@ do_validate_one() { error "'%s': name '%s' contains invalid chars: '%s'\n" \ "${br2_ext}" "${br2_name//\$/\$\$}" "${n//\$/\$\$}" fi - if [ -n "${BR2_EXT_PATHS["${br2_name}"]}" ]; then + eval d="\"\${BR2_EXT_PATHS_${br2_name}}\"" + if [ -n "${d}" ]; then error "'%s': name '%s' is already used in '%s'\n" \ - "${br2_ext}" "${br2_name}" "${BR2_EXT_PATHS["${br2_name}"]}" + "${br2_ext}" "${br2_name}" "${d}" fi br2_desc="$(sed -r -e '/^desc: +(.*)$/!d; s//\1/' "${br2_ext}/external.desc")" if [ ! -f "${br2_ext}/external.mk" ]; then
@@ -105,8 +108,8 @@ do_validate_one() { # Register this br2-external tree BR2_EXT_NAMES+=( "${br2_name}" ) - BR2_EXT_PATHS["${br2_name}"]="${br2_ext}" - BR2_EXT_DESCS["${br2_name}"]="${br2_desc:-${br2_name}}" + eval BR2_EXT_PATHS_${br2_name}="\"\${br2_ext}\"" + eval BR2_EXT_DESCS_${br2_name}="\"\${br2_desc:-\${br2_name}}\"" } # Generate the .mk snippet that defines makefile variables
@@ -117,13 +120,10 @@ do_mk() { printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - # We can't use ${BR2_EXT_NAMES[@]} directly: it is not guaranteed - # to be in the order paths were added (because it is an associative - # array). So we need to iterate on BR2_EXT_NAMES, which is sorted - # in the order names were added (because it is an indexed array). printf 'BR2_EXTERNAL ?=' for br2_name in "${BR2_EXT_NAMES[@]}"; do - printf ' %s' "${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" + printf ' %s' "${br2_ext}" done printf '\n'
@@ -138,8 +138,8 @@ do_mk() { fi for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_desc="\"\${BR2_EXT_DESCS_${br2_name}}\"" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" printf '\n' printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}" printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
@@ -165,8 +165,8 @@ do_kconfig() { printf '\n' for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_desc="\"\${BR2_EXT_DESCS_${br2_name}}\"" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then printf 'menu "%s"\n' "${br2_desc}" fi
Signature · 1 line
2.7.4
PATCH 1/1 core/br2-external: restore compatibility with old distros
Quoted 8 lines · Yann E. MORIN
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
We restore compatibility with those oldish distros using 'eval' to emulate associative arrays, as suggested by Arnout.
Quoted 6 lines
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>
Below some things I considered while reviewing.
Quoted 9 lines
--- support/scripts/br2-external | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)support/scripts/br2-external +0
-0@@ -1,10 +1,12 @@
[snip]
support/scripts/br2-external do_validate_one() { · line 96
Quoted 6 lines
support/scripts/br2-external +1
-1@@ -91,9 +93,10 @@ do_validate_one() { error "'%s': name '%s' contains invalid chars: '%s'\n" \ "${br2_ext}" "${br2_name//\$/\$\$}" "${n//\$/\$\$}" fi - if [ -n "${BR2_EXT_PATHS["${br2_name}"]}" ]; then + eval d="\"\${BR2_EXT_PATHS_${br2_name}}\""
Just for bikeshedding comparison:
eval d="\"\${BR2_EXT_PATHS_${br2_name}}\""
eval d='"${BR2_EXT_PATHS_'${br2_name}'}"'
Your version is better :-)
Quoted 8 lines
support/scripts/br2-external +2
-1+ if [ -n "${d}" ]; then error "'%s': name '%s' is already used in '%s'\n" \ - "${br2_ext}" "${br2_name}" "${BR2_EXT_PATHS["${br2_name}"]}" + "${br2_ext}" "${br2_name}" "${d}" fi br2_desc="$(sed -r -e '/^desc: +(.*)$/!d; s//\1/' "${br2_ext}/external.desc")" if [ ! -f "${br2_ext}/external.mk" ]; then
support/scripts/br2-external do_validate_one() { · line 112
Quoted 7 lines
support/scripts/br2-external +2
-2@@ -105,8 +108,8 @@ do_validate_one() { # Register this br2-external tree BR2_EXT_NAMES+=( "${br2_name}" ) - BR2_EXT_PATHS["${br2_name}"]="${br2_ext}" - BR2_EXT_DESCS["${br2_name}"]="${br2_desc:-${br2_name}}" + eval BR2_EXT_PATHS_${br2_name}="\"\${br2_ext}\"" + eval BR2_EXT_DESCS_${br2_name}="\"\${br2_desc:-\${br2_name}}\""
And another comparison:
eval BR2_EXT_PATHS_${br2_name}="\"\${br2_ext}\""
eval BR2_EXT_PATHS_${br2_name}='"${br2_ext}"'
On its own, the version with single quotes is better here, but your version is
more consistent with the other eval lines.
support/scripts/br2-external do_mk() { · line 120
Quoted 8 lines
support/scripts/br2-external +0
-1} # Generate the .mk snippet that defines makefile variables@@ -117,13 +120,10 @@ do_mk() { printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - # We can't use ${BR2_EXT_NAMES[@]} directly: it is not guaranteed
This comment was already wrong, it should have been BR2_EXT_PATHS. Without the
associative arrays, the option of using ${BR2_EXT_PATHS[@]} is no longer available, that's why the comment can be removed.
Perhaps the sentence above should have been part of the commit message :-)
Regards,
Arnout
Quoted 34 lines
support/scripts/br2-external +6
-8- # to be in the order paths were added (because it is an associative - # array). So we need to iterate on BR2_EXT_NAMES, which is sorted - # in the order names were added (because it is an indexed array). printf 'BR2_EXTERNAL ?=' for br2_name in "${BR2_EXT_NAMES[@]}"; do - printf ' %s' "${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" + printf ' %s' "${br2_ext}" done printf '\n'@@ -138,8 +138,8 @@ do_mk() { fi for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_desc="\"\${BR2_EXT_DESCS_${br2_name}}\"" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" printf '\n' printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}" printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"@@ -165,8 +165,8 @@ do_kconfig() { printf '\n' for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + eval br2_desc="\"\${BR2_EXT_DESCS_${br2_name}}\"" + eval br2_ext="\"\${BR2_EXT_PATHS_${br2_name}}\"" if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then printf 'menu "%s"\n' "${br2_desc}" fi
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
PATCH 1/1 core/br2-external: restore compatibility with old distros
In reply to Yann E. MORINHi Yann,
Since eval is kind of icky, how about something like this patch instead?
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
Remove the arrays by reading the external.desc files as we generate the output file.
--- support/scripts/br2-external | 94 +++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 41 deletions(-)
support/scripts/br2-external +53 -41
@@ -1,11 +1,6 @@ #!/bin/bash set -e -# The names and locations of the br2-external trees, once validated. -declare -a BR2_EXT_NAMES -declare -A BR2_EXT_PATHS -declare -A BR2_EXT_DESCS - # URL to manual for help in converting old br2-external trees. # Escape '#' so that make does not consider it a comment. MANUAL_URL='https://buildroot.org/manual.html\#br2-external-converting'
@@ -38,14 +33,15 @@ main() { exec >"${ofile}" - do_validate ${@//:/ } + # Split $PATH style argument into multiple arguments + set -- ${@//:/ } - do_${ofmt} + do_validate "${@}" + + do_${ofmt} "${@}" } -# Validates the br2-external trees passed as arguments. Makes each of -# them canonical and store them in the global arrays BR2_EXT_NAMES -# and BR2_EXT_PATHS. +# Validates the br2-external trees passed as arguments. # # Note: since this script is always first called from Makefile context # to generate the Makefile fragment before it is called to generate the
@@ -61,14 +57,16 @@ do_validate() { return fi - for br2_ext in "${@}"; do + for br2_ext; do do_validate_one "${br2_ext}" done + + validate_unique_names "${@}" } do_validate_one() { local br2_ext="${1}" - local br2_name br2_desc n + local br2_name n if [ ! -d "${br2_ext}" ]; then error "'%s': no such file or directory\n" "${br2_ext}"
@@ -80,7 +78,7 @@ do_validate_one() { error "'%s': does not have a name (in 'external.desc'). See %s\n" \ "${br2_ext}" "${MANUAL_URL}" fi - br2_name="$(sed -r -e '/^name: +(.*)$/!d; s//\1/' "${br2_ext}/external.desc")" + br2_name="$(get_field "${br2_ext}" 'name')" if [ -z "${br2_name}" ]; then error "'%s/external.desc': does not define the name\n" "${br2_ext}" fi
@@ -91,55 +89,69 @@ do_validate_one() { error "'%s': name '%s' contains invalid chars: '%s'\n" \ "${br2_ext}" "${br2_name//\$/\$\$}" "${n//\$/\$\$}" fi - if [ -n "${BR2_EXT_PATHS["${br2_name}"]}" ]; then - error "'%s': name '%s' is already used in '%s'\n" \ - "${br2_ext}" "${br2_name}" "${BR2_EXT_PATHS["${br2_name}"]}" - fi - br2_desc="$(sed -r -e '/^desc: +(.*)$/!d; s//\1/' "${br2_ext}/external.desc")" if [ ! -f "${br2_ext}/external.mk" ]; then error "'%s/external.mk': no such file or directory\n" "${br2_ext}" fi if [ ! -f "${br2_ext}/Config.in" ]; then error "'%s/Config.in': no such file or directory\n" "${br2_ext}" fi +} - # Register this br2-external tree - BR2_EXT_NAMES+=( "${br2_name}" ) - BR2_EXT_PATHS["${br2_name}"]="${br2_ext}" - BR2_EXT_DESCS["${br2_name}"]="${br2_desc:-${br2_name}}" +validate_unique_names() { + # If there are any duplicate names across the external.desc files, + # generate_dupes will output at least two lines of the form: + # <br2_ext_1> name + # <br2_ext_2> name + # If there is more that one duplicate, extra lines will be output but + # we do not include them in the error message due to formatting limitations + # of the error message. Once one duplicate is fixed, subsequent runs will + # identify the next duplicate(s). + set -- $(generate_dupes "${@}") + if [ ${#} -ne 0 ]; then + error "'%s': name '%s' is already used in '%s'\n" \ + "${1}" "${2}" "${3}" + fi +} + +generate_dupes() { + for br2_ext; do + printf '%s %s\n' ${br2_ext} "$(get_field "${br2_ext}" 'name')" + done | sort -k2,2 | uniq --skip-fields=1 --all-repeated +} + +# Get a field by name from an external.desc file +get_field() { + local br2_ext="${1}" + local field="${2}" + + sed -n -r -e "s/^${field}"': +(.*)$/\1/p' "${br2_ext}/external.desc" } # Generate the .mk snippet that defines makefile variables # for the br2-external tree do_mk() { - local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - # We can't use ${BR2_EXT_NAMES[@]} directly: it is not guaranteed - # to be in the order paths were added (because it is an associative - # array). So we need to iterate on BR2_EXT_NAMES, which is sorted - # in the order names were added (because it is an indexed array). printf 'BR2_EXTERNAL ?=' - for br2_name in "${BR2_EXT_NAMES[@]}"; do - printf ' %s' "${BR2_EXT_PATHS["${br2_name}"]}" - done + printf ' %s' "${@}" printf '\n' printf 'BR2_EXTERNAL_NAMES = \n' printf 'BR2_EXTERNAL_DIRS = \n' printf 'BR2_EXTERNAL_MKS = \n' - if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then + if [ ${#} -eq 0 ]; then printf '\n' printf '# No br2-external tree defined.\n' return fi - for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + for br2_ext; do + br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" printf '\n' printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}" printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"
@@ -151,12 +163,12 @@ do_mk() { # Generate the kconfig snippet for the br2-external tree. do_kconfig() { - local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then + if [ ${#} -eq 0 ]; then printf '# No br2-external tree defined.\n' return fi
@@ -164,10 +176,10 @@ do_kconfig() { printf 'menu "External options"\n' printf '\n' - for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + for br2_ext; do + br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" + if [ ${#} -gt 1 ]; then printf 'menu "%s"\n' "${br2_desc}" fi printf 'comment "%s (in %s)"\n' "${br2_desc}" "${br2_ext}"
@@ -175,7 +187,7 @@ do_kconfig() { printf '\tstring\n' printf '\tdefault "%s"\n' "${br2_ext}" printf 'source "%s/Config.in"\n' "${br2_ext}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + if [ ${#} -gt 1 ]; then printf 'endmenu # %s\n' "${br2_name}" fi printf '\n'
Signature · 1 line
2.1.4
PATCH 1/1 core/br2-external: restore compatibility with old distros
Cam,
Those first lines of the commit message should be placed after the line with '---', because 'git am' would automatically remove them.
There is also a duplicated SoB.
No need to resend because of these, it can be manually fixed when applying.
I didn't review the code, I only tested it.
Quoted 13 lines · Cam Hutchison
Hi Yann,
Since eval is kind of icky, how about something like this patch instead?
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
Remove the arrays by reading the external.desc files as we generate the output file.
Quoted 8 lines
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Cam Hutchison" <[email protected]>Cc "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>Signed-off-by Cam Hutchison <[email protected]>
[Simple tests using CentOS 5, bash 3.2.25:
1. make defconfig (in-tree build configuration);
2. created 2 small br2-external with a package and a defconfig each and successfully configured out-of-tree build with multiple br2-external using both defconfigs and downloaded the sources of both packages.]
Regards,
Ricardo
PATCH 1/1 core/br2-external: restore compatibility with old distros
Thanks for the testing, Ricardo.
Noted about the workflow errors. I was just throwing this out for comment right now. I can spin it properly if needed, but as you say - also easily adjusted on commit.
One thing I wasn't sure about and forgot to comment on when I posted was the invocation of uniq(1). I'm using some GNU options and I'm not sure when they were added to GNU uniq and what version RHEL5 has.
Can you confirm that you can invoke without error:
$ uniq --skip-fields=1 --all-repeated
If not, the generate_dupes() function will need to be fixed.
Quoted 41 lines · Ricardo Martincoski
Cam,
Those first lines of the commit message should be placed after the line with '---', because 'git am' would automatically remove them.
There is also a duplicated SoB.
No need to resend because of these, it can be manually fixed when applying.I didn't review the code, I only tested it.
Quoted 13 lines · Cam Hutchison
Hi Yann,
Since eval is kind of icky, how about something like this patch instead?
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
Remove the arrays by reading the external.desc files as we generate the output file.
Quoted 8 lines
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Cam Hutchison" <[email protected]>Cc "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>Signed-off-by Cam Hutchison <[email protected]>Tested-by Ricardo Martincoski <[email protected]>[Simple tests using CentOS 5, bash 3.2.25:
1. make defconfig (in-tree build configuration);
2. created 2 small br2-external with a package and a defconfig each and successfully configured out-of-tree build with multiple br2-external using both defconfigs and downloaded the sources of both packages.]Regards,
Ricardo
PATCH 1/1 core/br2-external: restore compatibility with old distros
Cam,
Quoted 6 lines · Cam Hutchison
Thanks for the testing, Ricardo.
Noted about the workflow errors. I was just throwing this out for comment right now. I can spin it properly if needed, but as you say - also easily adjusted on commit.
Quoted 6 lines
One thing I wasn't sure about and forgot to comment on when I posted was the invocation of uniq(1). I'm using some GNU options and I'm not sure when they were added to GNU uniq and what version RHEL5 has.
Can you confirm that you can invoke without error:
$ uniq --skip-fields=1 --all-repeated
Sure.
[ricardo at centos5 ~]$ grep release /etc/issue CentOS release 5.11 (Final)
[ricardo at centos5 ~]$ uniq --version | grep uniq uniq (GNU coreutils) 5.97
[ricardo at centos5 ~]$ uniq --skip-fields=1 --all-repeated
No error message is generated and uniq is waiting for stdin.
[ricardo at centos5 ~]$ uniq --help
Usage: uniq [OPTION]... [INPUT [OUTPUT]]
Discard all but one of successive identical lines from INPUT (or standard input), writing to OUTPUT (or standard output).
Mandatory arguments to long options are mandatory for short options too.
-c, --count prefix lines by the number of occurrences
-d, --repeated only print duplicate lines
-D, --all-repeated[=delimit-method] print all duplicate lines
delimit-method={none(default),prepend,separate} Delimiting is done with blank lines.
-f, --skip-fields=N avoid comparing the first N fields -i, --ignore-case ignore differences in case when comparing -s, --skip-chars=N avoid comparing the first N characters -u, --unique only print unique lines -w, --check-chars=N compare no more than N characters in lines
--help display this help and exit
--version output version information and exit
A field is a run of whitespace, then non-whitespace characters. Fields are skipped before chars.
Report bugs to <[email protected]>.
[ricardo at centos5 ~]$
Quoted 3 lines
If not, the
generate_dupes()function will need to be fixed.
Regards,
Ricardo
PATCH 1/1 core/br2-external: restore compatibility with old distros
In reply to Cam HutchisonCam, All,
(Arnout, Thomas, question for you at the end)
On 2016-11-19 22:53 -0000, Cam Hutchison spake thusly:
Quoted 1 line
Since eval is kind of icky, how about something like this patch instead?
I don't like it much. It is very intrusive, when we are so close to the release...
You are entirely getting rid of arrays, even the indexed arrays, even though bash has had support for them for ages now, and even RHEL-4 (4!) has a bash that supports indexed arrays...
Getting rid of the indxed arrays complexifies the script quite a bit. Also, using a function to retrieve fields makes the code even yet a little bit more complex.
Indeed, the eval are icky, but they are pretty much contained.
Plus, see a few comments below...
Quoted 25 lines
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
Remove the arrays by reading the external.desc files as we generate the output file.
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Cam Hutchison" <[email protected]>Cc "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>Signed-off-by Cam Hutchison <[email protected]>--- support/scripts/br2-external | 94 +++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 41 deletions(-)support/scripts/br2-external +0
-0
support/scripts/br2-external do_validate() { · line 60
Quoted 5 lines
support/scripts/br2-external +1
-1@@ -61,14 +57,16 @@ do_validate() { return fi - for br2_ext in "${@}"; do + for br2_ext; do
I always find this notation to be puzzling. Yes, I know it uses the positional arguments, but I don;t like implicit behaviour; it always leads to maintenance burden.
Just keep the explicit iteration: for br2_ext in "${@}"; do
[--SNIP--]
support/scripts/br2-external do_validate_one() { · line 104
Quoted 5 lines
support/scripts/br2-external +5
-0+validate_unique_names() { + # If there are any duplicate names across the external.desc files, + # generate_dupes will output at least two lines of the form: + # <br2_ext_1> name + # <br2_ext_2> name
In fact, the full output would be something like:
br2-ext-1 name-A
br2-ext-2 name-A
br2-ext-3 name-A
br2-ext-4 name-B
br2-ext-5 name-Bsupport/scripts/br2-external do_validate_one() { · line 105
Quoted 1 line
support/scripts/br2-external +1
-0+ # If there is more that one duplicate, extra lines will be output but
At first, I was not sure what you meant here. But you meant that, in the example above, br2-ext-3 will not be reported, and neither would br2-ext-4 and br2-ext-5.
(That's OK, the current code does not report them either).
support/scripts/br2-external do_validate_one() { · line 115
Quoted 10 lines
support/scripts/br2-external +10
-0+ # we do not include them in the error message due to formatting limitations + # of the error message. Once one duplicate is fixed, subsequent runs will + # identify the next duplicate(s). + set -- $(generate_dupes "${@}") + if [ ${#} -ne 0 ]; then + error "'%s': name '%s' is already used in '%s'\n" \ + "${1}" "${2}" "${3}" + fi +} +
The documentation of the output of generate_dupes should go there:
# ...here.
support/scripts/br2-external do_validate_one() { · line 127
Quoted 12 lines
support/scripts/br2-external +12
-0+generate_dupes() { + for br2_ext; do + printf '%s %s\n' ${br2_ext} "$(get_field "${br2_ext}" 'name')" + done | sort -k2,2 | uniq --skip-fields=1 --all-repeated +} + +# Get a field by name from an external.desc file +get_field() { + local br2_ext="${1}" + local field="${2}" + + sed -n -r -e "s/^${field}"': +(.*)$/\1/p' "${br2_ext}/external.desc"
Mixing quotes is always tricky... ;-) I try to avoid it when possible:
"s/^${field}: +(.*)\$/\\1/p"Quoted 11 lines
}
# Generate the .mk snippet that defines makefile variables
# for the br2-external tree do_mk() {support/scripts/br2-external +1
-1- local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n'
support/scripts/br2-external do_validate_one() { · line 140
Quoted 10 lines
support/scripts/br2-external +1
-7- # We can't use ${BR2_EXT_NAMES[@]} directly: it is not guaranteed - # to be in the order paths were added (because it is an associative - # array). So we need to iterate on BR2_EXT_NAMES, which is sorted - # in the order names were added (because it is an indexed array). printf 'BR2_EXTERNAL ?=' - for br2_name in "${BR2_EXT_NAMES[@]}"; do - printf ' %s' "${BR2_EXT_PATHS["${br2_name}"]}" - done + printf ' %s' "${@}" printf '\n'
You can do that in a single command:
printf 'BR2_EXTERNAL ?= %s\n' "${*}"Quoted 6 lines
printf '
BR2_EXTERNAL_NAMES= \n'
printf 'BR2_EXTERNAL_DIRS= \n'
printf 'BR2_EXTERNAL_MKS= \n'support/scripts/br2-external +0
-1- if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then
support/scripts/br2-external do_validate_one() { · line 152
Quoted 10 lines
support/scripts/br2-external +2
-3+ if [ ${#} -eq 0 ]; then printf '\n' printf '# No br2-external tree defined.\n' return fi - for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + for br2_ext; do
Please, keep explicit listing of positional arguments.
Quoted 21 lines
support/scripts/br2-external +4
-2+ br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" printf '\n' printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}" printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"@@ -151,12 +163,12 @@ do_mk() { # Generate the kconfig snippet for the br2-external tree. do_kconfig() { - local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then + if [ ${#} -eq 0 ]; then printf '# No br2-external tree defined.\n' return fi
support/scripts/br2-external do_kconfig() { · line 179
Quoted 8 lines
support/scripts/br2-external +1
-4@@ -164,10 +176,10 @@ do_kconfig() { printf 'menu "External options"\n' printf '\n' - for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + for br2_ext; do
Please, keep explicit listing of positional arguments.
However, I'm not sure you should fix and respin, I think this patch is a little bit too intrusive and makes the code a little bit more complex than really needed.
Yet, let's see what others think. Arnout, Thomas?
Regards,
Yann E. MORIN.
Quoted 22 lines
support/scripts/br2-external +4
-1+ br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" + if [ ${#} -gt 1 ]; then printf 'menu "%s"\n' "${br2_desc}" fi printf 'comment "%s (in %s)"\n' "${br2_desc}" "${br2_ext}"@@ -175,7 +187,7 @@ do_kconfig() { printf '\tstring\n' printf '\tdefault "%s"\n' "${br2_ext}" printf 'source "%s/Config.in"\n' "${br2_ext}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + if [ ${#} -gt 1 ]; then printf 'endmenu # %s\n' "${br2_name}" fi printf '\n'Signature · 6 lines
2.1.4 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH 1/1 core/br2-external: restore compatibility with old distros
Yann,
Thanks for the review.
I often said in my last job that if you get five people to review your shell script, you'll get ten opinions. Something about shell that gives such a diversity of views.
Quoted 2 lines · Yann E. MORIN
I don't like it much. It is very intrusive, when we are so close to the release...
It's a fair point but the script is small enough that it can easily be tested with a variety of inputs to validate it, which I did. There was a one character difference between the original script and this one in one test case but that was not significant.
Quoted 3 lines
You are entirely getting rid of arrays, even the indexed arrays, even though bash has had support for them for ages now, and even RHEL-4 (4!) has a bash that supports indexed arrays...
Sure. I was presenting a solution that removed the arrays altogether. The approach I took did not need any arrays, so while the direct problem was the associative arrays, just getting rid of them and leaving an indexed array did not make any sense.
Quoted 1 line
Getting rid of the indxed arrays complexifies the script quite a bit.
Really? To me this is so much simpler. The state has been removed, and state is always an additional cognitive and testing burden. The script functions become functional in that their output is a product of their input. This makes testing simpler and reduces the scope you need to keep in your head to understand a function.
Quoted 2 lines
Also, using a function to retrieve fields makes the code even yet a little bit more complex.
Factoring into smaller functions like this is both a benefit and a hindrance. It makes the initial reading of the code a bit harder as you need to understand the abstractions, but once understood it makes it easier. Since the function is a single line, it is easy to verify but since it also reduces repetition the complexity is reduced.
Quoted 33 lines
Indeed, the eval are icky, but they are pretty much contained.
Plus, see a few comments below...
Quoted 28 lines
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
Remove the arrays by reading the external.desc files as we generate the output file.
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Cam Hutchison" <[email protected]>Cc "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>Signed-off-by Cam Hutchison <[email protected]>--- support/scripts/br2-external | 94 +++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 41 deletions(-)support/scripts/br2-external +0
-0@@ -61,14 +57,16 @@ do_validate() { return fi
Quoted 8 lines
support/scripts/br2-external do_validate() { · line 60
Quoted 2 lines
support/scripts/br2-external +1
-1- for br2_ext in "${@}"; do + for br2_ext; doI always find this notation to be puzzling. Yes, I know it uses the positional arguments, but I don;t like implicit behaviour; it always leads to maintenance burden.
Just keep the explicit iteration: for
br2_extin "${@}"; do
I can't disagree more with this. In the expanded form, there are four ways you can iterate arguments of which only one is correct. I have seen the incorrect ways too often that now when I review code, I tell people to use the form I use here. The four ways are:
for var in $*
for var in "$*"
for var in $@
for var in "$@"
Not many people understand the different expansions here that I avoid it altogether. "for var" is valid shell and is *more explicit* in a way because it means only one thing - iterate over the positional parameters, correctly maintaining word boundaries. You can't get the quotes wrong, you can't use the wrong expansion and you can't accidentally drop in an extra argument from a typo.
Quoted 8 lines
[--SNIP--]
support/scripts/br2-external do_validate_one() { · line 104
Quoted 5 lines
support/scripts/br2-external +5
-0+validate_unique_names() { + # If there are any duplicate names across the external.desc files, + # generate_dupes will output at least two lines of the form: + # <br2_ext_1> name + # <br2_ext_2> name
Quoted 7 lines
In fact, the full output would be something like:
br2-ext-1 name-A br2-ext-2 name-A br2-ext-3 name-A br2-ext-4 name-B br2-ext-5 name-B
Yes, and they are both the same *form*. And I did say "at least two lines". The extra verbiage is not relevant so I omitted it.
Quoted 9 lines
support/scripts/br2-external do_validate_one() { · line 105
Quoted 2 lines
support/scripts/br2-external +1
-0+ # If there is more that one duplicate, extra lines will be output butAt first, I was not sure what you meant here. But you meant that, in the example above, br2-ext-3 will not be reported, and neither would br2-ext-4 and br2-ext-5.
(That's OK, the current code does not report them either).
Quoted 12 lines
support/scripts/br2-external do_validate_one() { · line 115
Quoted 10 lines
support/scripts/br2-external +10
-0+ # we do not include them in the error message due to formatting limitations + # of the error message. Once one duplicate is fixed, subsequent runs will + # identify the next duplicate(s). + set -- $(generate_dupes "${@}") + if [ ${#} -ne 0 ]; then + error "'%s': name '%s' is already used in '%s'\n" \ + "${1}" "${2}" "${3}" + fi +} +The documentation of the output of
generate_dupesshould go there:
I did put it there at first, but what I found in the end was that how the output was used was more important to understand. I could put a "head -n 2" in generate_dupes and then move the comment there which would also make some sense.
Quoted 17 lines
# ...here.support/scripts/br2-external do_validate_one() { · line 127
Quoted 12 lines
support/scripts/br2-external +12
-0+generate_dupes() { + for br2_ext; do + printf '%s %s\n' ${br2_ext} "$(get_field "${br2_ext}" 'name')" + done | sort -k2,2 | uniq --skip-fields=1 --all-repeated +} + +# Get a field by name from an external.desc file +get_field() { + local br2_ext="${1}" + local field="${2}" + + sed -n -r -e "s/^${field}"': +(.*)$/\1/p' "${br2_ext}/external.desc"Mixing quotes is always tricky... ;-) I try to avoid it when possible:
Quoted 1 line
"s/^${field}: +(.*)\$/\\1/p"Yeah. There's no sanity here. I hate backslash-quoting things that if I can put in the right quotes to avoid it, I will. The mashed-together double- and single-quote looks a lot more confusing now I see it in my mail client rather than my editor.
Quoted 23 lines
Quoted 12 lines
}
# Generate the .mk snippet that defines makefile variables
# for the br2-external tree do_mk() {support/scripts/br2-external +1
-1- local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n'support/scripts/br2-external do_validate_one() { · line 140
Quoted 10 lines
support/scripts/br2-external +1
-7- # We can't use ${BR2_EXT_NAMES[@]} directly: it is not guaranteed - # to be in the order paths were added (because it is an associative - # array). So we need to iterate on BR2_EXT_NAMES, which is sorted - # in the order names were added (because it is an indexed array). printf 'BR2_EXTERNAL ?=' - for br2_name in "${BR2_EXT_NAMES[@]}"; do - printf ' %s' "${BR2_EXT_PATHS["${br2_name}"]}" - done + printf ' %s' "${@}" printf '\n'
Quoted 3 lines
You can do that in a single command:
printf 'BR2_EXTERNAL ?= %s\n' "${*}"
Ack. Good call.
Quoted 13 lines
Quoted 13 lines
printf '
BR2_EXTERNAL_NAMES= \n'
printf 'BR2_EXTERNAL_DIRS= \n'
printf 'BR2_EXTERNAL_MKS= \n'support/scripts/br2-external +1
-1- if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then + if [ ${#} -eq 0 ]; then printf '\n' printf '# No br2-external tree defined.\n' return fi
Quoted 6 lines
support/scripts/br2-external do_validate_one() { · line 152
Quoted 4 lines
support/scripts/br2-external +1
-3- for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" + for br2_ext; doPlease, keep explicit listing of positional arguments.
Ack. See rationale above.
Quoted 25 lines
Quoted 25 lines
support/scripts/br2-external +4
-2+ br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" printf '\n' printf 'BR2_EXTERNAL_NAMES += %s\n' "${br2_name}" printf 'BR2_EXTERNAL_DIRS += %s\n' "${br2_ext}"@@ -151,12 +163,12 @@ do_mk() { # Generate the kconfig snippet for the br2-external tree. do_kconfig() { - local br2_name br2_ext + local br2_ext br2_name br2_desc printf '#\n# Automatically generated file; DO NOT EDIT.\n#\n' printf '\n' - if [ ${#BR2_EXT_NAMES[@]} -eq 0 ]; then + if [ ${#} -eq 0 ]; then printf '# No br2-external tree defined.\n' return fi@@ -164,10 +176,10 @@ do_kconfig() { printf 'menu "External options"\n' printf '\n'
Quoted 7 lines
support/scripts/br2-external do_kconfig() { · line 179
Quoted 5 lines
support/scripts/br2-external +1
-4- for br2_name in "${BR2_EXT_NAMES[@]}"; do - br2_desc="${BR2_EXT_DESCS["${br2_name}"]}" - br2_ext="${BR2_EXT_PATHS["${br2_name}"]}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + for br2_ext; doPlease, keep explicit listing of positional arguments.
Ack. See rationale above.
Quoted 6 lines
However, I'm not sure you should fix and respin, I think this patch is a little bit too intrusive and makes the code a little bit more complex than really needed.
Yet, let's see what others think. Arnout, Thomas?
I've got no attachment to this - to me, this code was simpler so I threw it out there (and I hate eval so much - forget trying to quote sanely with eval). Up to you guys.
Quoted 34 lines
Regards,
Yann E. MORIN.Quoted 18 lines
support/scripts/br2-external +4
-2+ br2_name="$(get_field "${br2_ext}" 'name')" + br2_desc="$(get_field "${br2_ext}" 'desc')" + if [ ${#} -gt 1 ]; then printf 'menu "%s"\n' "${br2_desc}" fi printf 'comment "%s (in %s)"\n' "${br2_desc}" "${br2_ext}"@@ -175,7 +187,7 @@ do_kconfig() { printf '\tstring\n' printf '\tdefault "%s"\n' "${br2_ext}" printf 'source "%s/Config.in"\n' "${br2_ext}" - if [ ${#BR2_EXT_NAMES[@]} -gt 1 ]; then + if [ ${#} -gt 1 ]; then printf 'endmenu # %s\n' "${br2_name}" fi printf '\n' --2.1.4
Quoted 4 lines
_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot--
.-----------------.--------------------.------------------.--------------------.| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |'------------------------------^-------^------------------^--------------------'
PATCH 1/1 core/br2-external: restore compatibility with old distros
In reply to Yann E. MORINYann,
Quoted 8 lines · Yann E. MORIN
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
We restore compatibility with those oldish distros using 'eval' to emulate associative arrays, as suggested by Arnout.
Quoted 6 lines
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>
[Simple tests using CentOS 5, bash 3.2.25:
1. make defconfig (in-tree build configuration);
2. created 2 small br2-external with a package and a defconfig each and successfully configured out-of-tree build with multiple br2-external using both defconfigs and downloaded the sources of both packages.]
Regards,
Ricardo
Thomas Petazzoni
AppliedPATCH 1/1 core/br2-external: restore compatibility with old distros
In reply to Yann E. MORINHello,
Quoted 14 lines · Yann E. MORIN
Currently, the br2-external script uses bash-4's associative arrays.
However, some oldish enterprise-class distros like RHEL5 still use bash-3.1 which lacks associative arrays.
We restore compatibility with those oldish distros using 'eval' to emulate associative arrays, as suggested by Arnout.
Reported-by Ricardo Martincoski <[email protected]>Signed-off-by "Yann E. MORIN" <[email protected]>Cc Ricardo Martincoski <[email protected]>Cc Thomas De Schampheleire <[email protected]>Cc Arnout Vandecappelle <[email protected]>Cc Max Filippov <[email protected]>
Quoted 3 lines
--- support/scripts/br2-external | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Applied to master, thanks.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com