diff mbox

[devtools] makechrootpkg: Avoid having code floating around outside of a function.

Message ID 20170405223935.13035-2-lukeshu@lukeshu.com
State Superseded
Headers show

Commit Message

Luke Shumaker April 5, 2017, 10:39 p.m. UTC
From: Luke Shumaker <lukeshu@parabola.nu>

This means wrapping variable initialization in init_variables(), and the
main program routine in main().

I did NOT put `shopt -s nullglob` in to a function.
---
 makechrootpkg.in | 252 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 130 insertions(+), 122 deletions(-)

Comments

Dave Reisner April 5, 2017, 10:42 p.m. UTC | #1
On Wed, Apr 05, 2017 at 06:39:35PM -0400, lukeshu@lukeshu.com wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> This means wrapping variable initialization in init_variables(), and the
> main program routine in main().
> 
> I did NOT put `shopt -s nullglob` in to a function.
> ---
>  makechrootpkg.in | 252 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 130 insertions(+), 122 deletions(-)
> 
> diff --git a/makechrootpkg.in b/makechrootpkg.in
> index f6764cb..93cdc10 100644
> --- a/makechrootpkg.in
> +++ b/makechrootpkg.in
> @@ -15,26 +15,28 @@ m4_include(lib/archroot.sh)
>  
>  shopt -s nullglob
>  
> -default_makepkg_args=(-s --noconfirm -L --holdver)
> -makepkg_args=("${default_makepkg_args[@]}")
> -repack=false
> -update_first=false
> -clean_first=false
> -run_namcap=false
> -temp_chroot=false
> -chrootdir=
> -passeddir=
> -makepkg_user=
> -declare -a install_pkgs
> -declare -i ret=0
> -
> -bindmounts_ro=()
> -bindmounts_rw=()
> -
> -copy=$USER
> -[[ -n ${SUDO_USER:-} ]] && copy=$SUDO_USER
> -[[ -z "$copy" || $copy = root ]] && copy=copy
> -src_owner=${SUDO_USER:-$USER}
> +init_variables() {
> +	default_makepkg_args=(-s --noconfirm -L --holdver)
> +	makepkg_args=("${default_makepkg_args[@]}")
> +	repack=false
> +	update_first=false
> +	clean_first=false
> +	run_namcap=false
> +	temp_chroot=false
> +	chrootdir=
> +	passeddir=
> +	makepkg_user=
> +	declare -a install_pkgs
> +	declare -i ret=0

By using declare here, you're scoping the variables to the function.

> +
> +	bindmounts_ro=()
> +	bindmounts_rw=()
> +
> +	copy=$USER
> +	[[ -n ${SUDO_USER:-} ]] && copy=$SUDO_USER
> +	[[ -z "$copy" || $copy = root ]] && copy=copy
> +	src_owner=${SUDO_USER:-$USER}
> +}
>  
>  usage() {
>  	echo "Usage: ${0##*/} [options] -r <chrootdir> [--] [makepkg args]"
> @@ -308,109 +310,115 @@ move_products() {
>  }
>  # }}}
>  
> -while getopts 'hcur:I:l:nTD:d:U:' arg; do
> -	case "$arg" in
> -		c) clean_first=true ;;
> -		D) bindmounts_ro+=(--bind-ro="$OPTARG") ;;
> -		d) bindmounts_rw+=(--bind="$OPTARG") ;;
> -		u) update_first=true ;;
> -		r) passeddir="$OPTARG" ;;
> -		I) install_pkgs+=("$OPTARG") ;;
> -		l) copy="$OPTARG" ;;
> -		n) run_namcap=true; makepkg_args+=(-i) ;;
> -		T) temp_chroot=true; copy+="-$$" ;;
> -		U) makepkg_user="$OPTARG" ;;
> -		h|*) usage ;;
> -	esac
> -done
> -
> -[[ ! -f PKGBUILD && -z "${install_pkgs[*]}" ]] && die 'This must be run in a directory containing a PKGBUILD.'
> -[[ -n $makepkg_user && -z $(id -u "$makepkg_user") ]] && die 'Invalid makepkg user.'
> -
> -check_root
> -
> -# Canonicalize chrootdir, getting rid of trailing /
> -chrootdir=$(readlink -e "$passeddir")
> -[[ ! -d $chrootdir ]] && die "No chroot dir defined, or invalid path '%s'" "$passeddir"
> -[[ ! -d $chrootdir/root ]] && die "Missing chroot dir root directory. Try using: mkarchroot %s/root base-devel" "$chrootdir"
> -
> -if [[ ${copy:0:1} = / ]]; then
> -	copydir=$copy
> -else
> -	copydir="$chrootdir/$copy"
> -fi
> -
> -# Pass all arguments after -- right to makepkg
> -makepkg_args+=("${@:$OPTIND}")
> -
> -# See if -R was passed to makepkg
> -for arg in "${@:OPTIND}"; do
> -	case ${arg%%=*} in
> -		-*R*|--repackage)
> -			repack=true
> -			break 2
> -			;;
> -	esac
> -done
> -
> -if [[ -n $SUDO_USER ]]; then
> -	eval "USER_HOME=~$SUDO_USER"
> -else
> -	USER_HOME=$HOME
> -fi
> -
> -umask 0022
> -
> -load_vars "${XDG_CONFIG_HOME:-$USER_HOME/.config}/pacman/makepkg.conf" || load_vars "$USER_HOME/.makepkg.conf"
> -load_vars /etc/makepkg.conf
> -
> -# Use PKGBUILD directory if these don't exist
> -[[ -d $PKGDEST ]]    || PKGDEST=$PWD
> -[[ -d $SRCDEST ]]    || SRCDEST=$PWD
> -[[ -d $SRCPKGDEST ]] || SRCPKGDEST=$PWD
> -[[ -d $LOGDEST ]]    || LOGDEST=$PWD
> -
> -# Lock the chroot we want to use. We'll keep this lock until we exit.
> -lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
> -
> -if [[ ! -d $copydir ]] || $clean_first; then
> -	sync_chroot "$chrootdir" "$copy"
> -fi
> -
> -$update_first && arch-nspawn "$copydir" \
> +main() {
> +	init_variables
> +
> +	while getopts 'hcur:I:l:nTD:d:U:' arg; do
> +		case "$arg" in
> +			c) clean_first=true ;;
> +			D) bindmounts_ro+=(--bind-ro="$OPTARG") ;;
> +			d) bindmounts_rw+=(--bind="$OPTARG") ;;
> +			u) update_first=true ;;
> +			r) passeddir="$OPTARG" ;;
> +			I) install_pkgs+=("$OPTARG") ;;
> +			l) copy="$OPTARG" ;;
> +			n) run_namcap=true; makepkg_args+=(-i) ;;
> +			T) temp_chroot=true; copy+="-$$" ;;
> +			U) makepkg_user="$OPTARG" ;;
> +			h|*) usage ;;
> +		esac
> +	done
> +
> +	[[ ! -f PKGBUILD && -z "${install_pkgs[*]}" ]] && die 'This must be run in a directory containing a PKGBUILD.'
> +	[[ -n $makepkg_user && -z $(id -u "$makepkg_user") ]] && die 'Invalid makepkg user.'
> +
> +	check_root
> +
> +	# Canonicalize chrootdir, getting rid of trailing /
> +	chrootdir=$(readlink -e "$passeddir")
> +	[[ ! -d $chrootdir ]] && die "No chroot dir defined, or invalid path '%s'" "$passeddir"
> +	[[ ! -d $chrootdir/root ]] && die "Missing chroot dir root directory. Try using: mkarchroot %s/root base-devel" "$chrootdir"
> +
> +	if [[ ${copy:0:1} = / ]]; then
> +		copydir=$copy
> +	else
> +		copydir="$chrootdir/$copy"
> +	fi
> +
> +	# Pass all arguments after -- right to makepkg
> +	makepkg_args+=("${@:$OPTIND}")
> +
> +	# See if -R was passed to makepkg
> +	for arg in "${@:OPTIND}"; do
> +		case ${arg%%=*} in
> +			-*R*|--repackage)
> +				repack=true
> +				break 2
> +				;;
> +		esac
> +	done
> +
> +	if [[ -n $SUDO_USER ]]; then
> +		eval "USER_HOME=~$SUDO_USER"
> +	else
> +		USER_HOME=$HOME
> +	fi
> +
> +	umask 0022
> +
> +	load_vars "${XDG_CONFIG_HOME:-$USER_HOME/.config}/pacman/makepkg.conf" || load_vars "$USER_HOME/.makepkg.conf"
> +	load_vars /etc/makepkg.conf
> +
> +	# Use PKGBUILD directory if these don't exist
> +	[[ -d $PKGDEST ]]    || PKGDEST=$PWD
> +	[[ -d $SRCDEST ]]    || SRCDEST=$PWD
> +	[[ -d $SRCPKGDEST ]] || SRCPKGDEST=$PWD
> +	[[ -d $LOGDEST ]]    || LOGDEST=$PWD
> +
> +	# Lock the chroot we want to use. We'll keep this lock until we exit.
> +	lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
> +
> +	if [[ ! -d $copydir ]] || $clean_first; then
> +		sync_chroot "$chrootdir" "$copy"
> +	fi
> +
> +	$update_first && arch-nspawn "$copydir" \
> +			"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
> +			pacman -Syu --noconfirm
> +
> +	if [[ -n ${install_pkgs[*]:-} ]]; then
> +		install_packages "$copydir" "${install_pkgs[@]}"
> +		ret=$?
> +		# If there is no PKGBUILD we have done
> +		[[ -f PKGBUILD ]] || return $ret
> +	fi
> +
> +	download_sources "$copydir" "$src_owner"
> +
> +	prepare_chroot "$copydir" "$USER_HOME" "$repack"
> +
> +	if arch-nspawn "$copydir" \
> +		--bind="$PWD:/startdir" \
> +		--bind="$SRCDEST:/srcdest" \
>  		"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
> -		pacman -Syu --noconfirm
> +		/chrootbuild "${makepkg_args[@]}"
> +	then
> +		move_products "$copydir" "$src_owner"
> +	else
> +		(( ret += 1 ))
> +	fi
>  
> -if [[ -n ${install_pkgs[*]:-} ]]; then
> -	install_packages "$copydir" "${install_pkgs[@]}"
> -	ret=$?
> -	# If there is no PKGBUILD we have done
> -	[[ -f PKGBUILD ]] || return $ret
> -fi
> -
> -download_sources "$copydir" "$src_owner"
> -
> -prepare_chroot "$copydir" "$USER_HOME" "$repack"
> -
> -if arch-nspawn "$copydir" \
> -	--bind="$PWD:/startdir" \
> -	--bind="$SRCDEST:/srcdest" \
> -	"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
> -	/chrootbuild "${makepkg_args[@]}"
> -then
> -	move_products "$copydir" "$src_owner"
> -else
> -	(( ret += 1 ))
> -fi
> -
> -$temp_chroot && delete_chroot "$copydir" "$copy"
> -
> -if (( ret != 0 )); then
> -	if $temp_chroot; then
> -		die "Build failed"
> +	$temp_chroot && delete_chroot "$copydir" "$copy"
> +
> +	if (( ret != 0 )); then
> +		if $temp_chroot; then
> +			die "Build failed"
> +		else
> +			die "Build failed, check %s/build" "$copydir"
> +		fi
>  	else
> -		die "Build failed, check %s/build" "$copydir"
> +		true
>  	fi
> -else
> -	true
> -fi
> +}
> +
> +main "$@"
> -- 
> 2.12.1
Luke Shumaker April 5, 2017, 11:06 p.m. UTC | #2
> > -declare -a install_pkgs
> > -declare -i ret=0
> > +	declare -a install_pkgs
> > +	declare -i ret=0
> 
> By using declare here, you're scoping the variables to the function.

You are, of course, correct.  I'm submitting a revised patch that adds
the `-g` flag to `declare`.

Also, as I mention in the revised commit message, you may wish to get
rid of the init_variables() function, and move that bit into the
main() function; which would make the concern go away.
diff mbox

Patch

diff --git a/makechrootpkg.in b/makechrootpkg.in
index f6764cb..93cdc10 100644
--- a/makechrootpkg.in
+++ b/makechrootpkg.in
@@ -15,26 +15,28 @@  m4_include(lib/archroot.sh)
 
 shopt -s nullglob
 
-default_makepkg_args=(-s --noconfirm -L --holdver)
-makepkg_args=("${default_makepkg_args[@]}")
-repack=false
-update_first=false
-clean_first=false
-run_namcap=false
-temp_chroot=false
-chrootdir=
-passeddir=
-makepkg_user=
-declare -a install_pkgs
-declare -i ret=0
-
-bindmounts_ro=()
-bindmounts_rw=()
-
-copy=$USER
-[[ -n ${SUDO_USER:-} ]] && copy=$SUDO_USER
-[[ -z "$copy" || $copy = root ]] && copy=copy
-src_owner=${SUDO_USER:-$USER}
+init_variables() {
+	default_makepkg_args=(-s --noconfirm -L --holdver)
+	makepkg_args=("${default_makepkg_args[@]}")
+	repack=false
+	update_first=false
+	clean_first=false
+	run_namcap=false
+	temp_chroot=false
+	chrootdir=
+	passeddir=
+	makepkg_user=
+	declare -a install_pkgs
+	declare -i ret=0
+
+	bindmounts_ro=()
+	bindmounts_rw=()
+
+	copy=$USER
+	[[ -n ${SUDO_USER:-} ]] && copy=$SUDO_USER
+	[[ -z "$copy" || $copy = root ]] && copy=copy
+	src_owner=${SUDO_USER:-$USER}
+}
 
 usage() {
 	echo "Usage: ${0##*/} [options] -r <chrootdir> [--] [makepkg args]"
@@ -308,109 +310,115 @@  move_products() {
 }
 # }}}
 
-while getopts 'hcur:I:l:nTD:d:U:' arg; do
-	case "$arg" in
-		c) clean_first=true ;;
-		D) bindmounts_ro+=(--bind-ro="$OPTARG") ;;
-		d) bindmounts_rw+=(--bind="$OPTARG") ;;
-		u) update_first=true ;;
-		r) passeddir="$OPTARG" ;;
-		I) install_pkgs+=("$OPTARG") ;;
-		l) copy="$OPTARG" ;;
-		n) run_namcap=true; makepkg_args+=(-i) ;;
-		T) temp_chroot=true; copy+="-$$" ;;
-		U) makepkg_user="$OPTARG" ;;
-		h|*) usage ;;
-	esac
-done
-
-[[ ! -f PKGBUILD && -z "${install_pkgs[*]}" ]] && die 'This must be run in a directory containing a PKGBUILD.'
-[[ -n $makepkg_user && -z $(id -u "$makepkg_user") ]] && die 'Invalid makepkg user.'
-
-check_root
-
-# Canonicalize chrootdir, getting rid of trailing /
-chrootdir=$(readlink -e "$passeddir")
-[[ ! -d $chrootdir ]] && die "No chroot dir defined, or invalid path '%s'" "$passeddir"
-[[ ! -d $chrootdir/root ]] && die "Missing chroot dir root directory. Try using: mkarchroot %s/root base-devel" "$chrootdir"
-
-if [[ ${copy:0:1} = / ]]; then
-	copydir=$copy
-else
-	copydir="$chrootdir/$copy"
-fi
-
-# Pass all arguments after -- right to makepkg
-makepkg_args+=("${@:$OPTIND}")
-
-# See if -R was passed to makepkg
-for arg in "${@:OPTIND}"; do
-	case ${arg%%=*} in
-		-*R*|--repackage)
-			repack=true
-			break 2
-			;;
-	esac
-done
-
-if [[ -n $SUDO_USER ]]; then
-	eval "USER_HOME=~$SUDO_USER"
-else
-	USER_HOME=$HOME
-fi
-
-umask 0022
-
-load_vars "${XDG_CONFIG_HOME:-$USER_HOME/.config}/pacman/makepkg.conf" || load_vars "$USER_HOME/.makepkg.conf"
-load_vars /etc/makepkg.conf
-
-# Use PKGBUILD directory if these don't exist
-[[ -d $PKGDEST ]]    || PKGDEST=$PWD
-[[ -d $SRCDEST ]]    || SRCDEST=$PWD
-[[ -d $SRCPKGDEST ]] || SRCPKGDEST=$PWD
-[[ -d $LOGDEST ]]    || LOGDEST=$PWD
-
-# Lock the chroot we want to use. We'll keep this lock until we exit.
-lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
-
-if [[ ! -d $copydir ]] || $clean_first; then
-	sync_chroot "$chrootdir" "$copy"
-fi
-
-$update_first && arch-nspawn "$copydir" \
+main() {
+	init_variables
+
+	while getopts 'hcur:I:l:nTD:d:U:' arg; do
+		case "$arg" in
+			c) clean_first=true ;;
+			D) bindmounts_ro+=(--bind-ro="$OPTARG") ;;
+			d) bindmounts_rw+=(--bind="$OPTARG") ;;
+			u) update_first=true ;;
+			r) passeddir="$OPTARG" ;;
+			I) install_pkgs+=("$OPTARG") ;;
+			l) copy="$OPTARG" ;;
+			n) run_namcap=true; makepkg_args+=(-i) ;;
+			T) temp_chroot=true; copy+="-$$" ;;
+			U) makepkg_user="$OPTARG" ;;
+			h|*) usage ;;
+		esac
+	done
+
+	[[ ! -f PKGBUILD && -z "${install_pkgs[*]}" ]] && die 'This must be run in a directory containing a PKGBUILD.'
+	[[ -n $makepkg_user && -z $(id -u "$makepkg_user") ]] && die 'Invalid makepkg user.'
+
+	check_root
+
+	# Canonicalize chrootdir, getting rid of trailing /
+	chrootdir=$(readlink -e "$passeddir")
+	[[ ! -d $chrootdir ]] && die "No chroot dir defined, or invalid path '%s'" "$passeddir"
+	[[ ! -d $chrootdir/root ]] && die "Missing chroot dir root directory. Try using: mkarchroot %s/root base-devel" "$chrootdir"
+
+	if [[ ${copy:0:1} = / ]]; then
+		copydir=$copy
+	else
+		copydir="$chrootdir/$copy"
+	fi
+
+	# Pass all arguments after -- right to makepkg
+	makepkg_args+=("${@:$OPTIND}")
+
+	# See if -R was passed to makepkg
+	for arg in "${@:OPTIND}"; do
+		case ${arg%%=*} in
+			-*R*|--repackage)
+				repack=true
+				break 2
+				;;
+		esac
+	done
+
+	if [[ -n $SUDO_USER ]]; then
+		eval "USER_HOME=~$SUDO_USER"
+	else
+		USER_HOME=$HOME
+	fi
+
+	umask 0022
+
+	load_vars "${XDG_CONFIG_HOME:-$USER_HOME/.config}/pacman/makepkg.conf" || load_vars "$USER_HOME/.makepkg.conf"
+	load_vars /etc/makepkg.conf
+
+	# Use PKGBUILD directory if these don't exist
+	[[ -d $PKGDEST ]]    || PKGDEST=$PWD
+	[[ -d $SRCDEST ]]    || SRCDEST=$PWD
+	[[ -d $SRCPKGDEST ]] || SRCPKGDEST=$PWD
+	[[ -d $LOGDEST ]]    || LOGDEST=$PWD
+
+	# Lock the chroot we want to use. We'll keep this lock until we exit.
+	lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
+
+	if [[ ! -d $copydir ]] || $clean_first; then
+		sync_chroot "$chrootdir" "$copy"
+	fi
+
+	$update_first && arch-nspawn "$copydir" \
+			"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
+			pacman -Syu --noconfirm
+
+	if [[ -n ${install_pkgs[*]:-} ]]; then
+		install_packages "$copydir" "${install_pkgs[@]}"
+		ret=$?
+		# If there is no PKGBUILD we have done
+		[[ -f PKGBUILD ]] || return $ret
+	fi
+
+	download_sources "$copydir" "$src_owner"
+
+	prepare_chroot "$copydir" "$USER_HOME" "$repack"
+
+	if arch-nspawn "$copydir" \
+		--bind="$PWD:/startdir" \
+		--bind="$SRCDEST:/srcdest" \
 		"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
-		pacman -Syu --noconfirm
+		/chrootbuild "${makepkg_args[@]}"
+	then
+		move_products "$copydir" "$src_owner"
+	else
+		(( ret += 1 ))
+	fi
 
-if [[ -n ${install_pkgs[*]:-} ]]; then
-	install_packages "$copydir" "${install_pkgs[@]}"
-	ret=$?
-	# If there is no PKGBUILD we have done
-	[[ -f PKGBUILD ]] || return $ret
-fi
-
-download_sources "$copydir" "$src_owner"
-
-prepare_chroot "$copydir" "$USER_HOME" "$repack"
-
-if arch-nspawn "$copydir" \
-	--bind="$PWD:/startdir" \
-	--bind="$SRCDEST:/srcdest" \
-	"${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \
-	/chrootbuild "${makepkg_args[@]}"
-then
-	move_products "$copydir" "$src_owner"
-else
-	(( ret += 1 ))
-fi
-
-$temp_chroot && delete_chroot "$copydir" "$copy"
-
-if (( ret != 0 )); then
-	if $temp_chroot; then
-		die "Build failed"
+	$temp_chroot && delete_chroot "$copydir" "$copy"
+
+	if (( ret != 0 )); then
+		if $temp_chroot; then
+			die "Build failed"
+		else
+			die "Build failed, check %s/build" "$copydir"
+		fi
 	else
-		die "Build failed, check %s/build" "$copydir"
+		true
 	fi
-else
-	true
-fi
+}
+
+main "$@"