[pacman-dev,v2,2/3] makepkg: use the `declare` builtin when backing up variables to eval

Message ID 20180619202635.24426-2-eschwartz@archlinux.org
State New
Headers show
Series
  • [pacman-dev,v2,1/3] configure: bump the minimum version of bash to 4.4
Related show

Commit Message

Eli Schwartz June 19, 2018, 8:26 p.m. UTC
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(-)

Comments

Allan McRae June 20, 2018, 8:59 a.m. UTC | #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
Allan McRae June 21, 2018, 5:48 a.m. UTC | #2
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

Patch

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