[pacman-dev] [PATCH v2 1/3] configure: bump the minimum version of bash to 4.4
This is required in order to use declare -g and ${var@a} Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index fb9d18e1..6ba9d737 100644 --- a/configure.ac +++ b/configure.ac @@ -193,18 +193,18 @@ AC_DEFUN([AX_PROG_PERL_VERSION], AX_PROG_PERL_VERSION([5.10.1], [], [AC_MSG_ERROR([perl is too old])]) AS_IF([test "x$BASH_SHELL" = "xfalse"], - AC_MSG_WARN([*** bash >= 4.1.0 is required for pacman scripts]), + AC_MSG_WARN([*** bash >= 4.4.0 is required for pacman scripts]), [bash_version_major=`$BASH_SHELL -c 'echo "${BASH_VERSINFO[[0]]}"'` bash_version_minor=`$BASH_SHELL -c 'echo "${BASH_VERSINFO[[1]]}"'` ok=yes if test "$bash_version_major" -lt 4; then ok=no fi - if test "$bash_version_major" -eq 4 && test "$bash_version_minor" -lt 1; then + if test "$bash_version_major" -eq 4 && test "$bash_version_minor" -lt 4; then ok=no fi if test "$ok" = "no"; then - AC_MSG_ERROR([*** bash >= 4.1.0 is required for pacman scripts]) + AC_MSG_ERROR([*** bash >= 4.4.0 is required for pacman scripts]) fi unset bash_version_major bash_version_minor ok]) -- 2.17.1
This re-applies commit 9e52a36794552b77ecf26f7f34b226d096978f1e with fixes after reverting it in 10ca4f48311370cdd580f66096d5e94858fde467 for the maintenance release. The split package metadata backup/restore was initially refactored to use declare, which actually declares variables in a local scope when in a function. This did not play nicely with debug packages, which unset most metadata variables, thereby reverting to the global scope rather than resulting in unset metadata. This could be fixed by explicitly marking the variables as global, or, alternatively, by refactoring everything to use local function variables. However, the latter simply moves the issue to other areas (what happens if a user-defined package function uses unset instead of foo=() for the same effect). Now that the minimum version of bash has been raised, it is safe to once more apply (a working version of) this enhancement. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- v2: patchset now targets pacman 6.x and is rebased onto https://patchwork.archlinux.org/patch/631/ scripts/makepkg.sh.in | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index d35dd62d..5f351ef5 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1125,34 +1125,26 @@ check_build_status() { backup_package_variables() { local var for var in ${splitpkg_overrides[@]}; do - local indirect="${var}_backup" - eval "${indirect}=(\"\${$var[@]}\")" - done -} - -restore_package_variables() { - local var - for var in ${splitpkg_overrides[@]}; do - local indirect="${var}_backup" - if [[ -n ${!indirect} ]]; then - eval "${var}=(\"\${$indirect[@]}\")" + if [[ ${!var} ]]; then + printf '%s\n' "declare -g $var" + declare -p $var else - unset ${var} + printf '%s\n' "unset $var" fi done } run_split_packaging() { local pkgname_backup=("${pkgname[@]}") + local restore_package_variables="$(backup_package_variables)" for pkgname in ${pkgname_backup[@]}; do pkgdir="$pkgdirbase/$pkgname" mkdir "$pkgdir" - backup_package_variables run_package $pkgname tidy_install lint_package || exit $E_PACKAGE_FAILED create_package - restore_package_variables + eval "$restore_package_variables" done pkgname=("${pkgname_backup[@]}") create_debug_package -- 2.17.1
On 20/06/18 06:26, Eli Schwartz wrote:
This re-applies commit 9e52a36794552b77ecf26f7f34b226d096978f1e with fixes after reverting it in 10ca4f48311370cdd580f66096d5e94858fde467 for the maintenance release.
The split package metadata backup/restore was initially refactored to use declare, which actually declares variables in a local scope when in a function. This did not play nicely with debug packages, which unset most metadata variables, thereby reverting to the global scope rather than resulting in unset metadata.
This could be fixed by explicitly marking the variables as global, or, alternatively, by refactoring everything to use local function variables. However, the latter simply moves the issue to other areas (what happens if a user-defined package function uses unset instead of foo=() for the same effect).
Now that the minimum version of bash has been raised, it is safe to once more apply (a working version of) this enhancement.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
v2: patchset now targets pacman 6.x and is rebased onto https://patchwork.archlinux.org/patch/631/
Can you provide an example PKGBUILD that demonstrated the bug? Thanks, A
On 20/06/18 06:26, Eli Schwartz wrote:
This re-applies commit 9e52a36794552b77ecf26f7f34b226d096978f1e with fixes after reverting it in 10ca4f48311370cdd580f66096d5e94858fde467 for the maintenance release.
The split package metadata backup/restore was initially refactored to use declare, which actually declares variables in a local scope when in a function. This did not play nicely with debug packages, which unset most metadata variables, thereby reverting to the global scope rather than resulting in unset metadata.
This could be fixed by explicitly marking the variables as global, or, alternatively, by refactoring everything to use local function variables. However, the latter simply moves the issue to other areas (what happens if a user-defined package function uses unset instead of foo=() for the same effect).
Now that the minimum version of bash has been raised, it is safe to once more apply (a working version of) this enhancement.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
v2: patchset now targets pacman 6.x and is rebased onto https://patchwork.archlinux.org/patch/631/
So, this patch is also broken. Given the number of "fixed" versions that have been posted so far, I will not accept a patch touching this piece of code again unless it fixes a genuine bug. And given that no bugs have been found in that code in the nine years since the pacman-3.2 release it was introduced in... A
Now that we require bash 4.4 this is "more correct" than analyzing the output of declare -p to see if it compares favorably with -a. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- scripts/libmakepkg/util/util.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index e1ca5cb7..0fb89186 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -42,7 +42,7 @@ is_array() { local v=$1 local ret=1 - if [[ $(declare -p "$v") == declare\ -*([[:alnum:]])a*([[:alnum:]])\ * ]]; then + if [[ ${!v@a} = *a* ]]; then ret=0 fi -- 2.17.1
On Tue, 19 Jun 2018 16:26:33 -0400, Eli Schwartz wrote:
This is required in order to use declare -g and ${var@a}
Huh? It's needed for @a, but wasn't `declare -g` in Bash 4.2? -- Happy hacking, ~ Luke Shumaker
On 07/12/2018 01:30 AM, Luke Shumaker wrote:
On Tue, 19 Jun 2018 16:26:33 -0400, Eli Schwartz wrote:
This is required in order to use declare -g and ${var@a}
Huh? It's needed for @a, but wasn't `declare -g` in Bash 4.2?
We moved from bash 4.1, it's quite genuine to say that moving from bash 4.1 to bash 4.4 enables using declare -g. -- Eli Schwartz Bug Wrangler and Trusted User
participants (3)
-
Allan McRae
-
Eli Schwartz
-
Luke Shumaker