[pacman-dev] libmakepkg/executable: don't rely on scoped value of $ret to flag outcomes

Message ID 20181128040036.2612-1-eschwartz@archlinux.org
State Accepted, archived
Headers show
Series
  • [pacman-dev] libmakepkg/executable: don't rely on scoped value of $ret to flag outcomes
Related show

Commit Message

Eli Schwartz Nov. 28, 2018, 4 a.m. UTC
Elsewhere, we return 1 if a library dropin fails, and when running
functions in a loop, we use `|| ret=1` to preserve scope. This ensures
the return value of the function remains useful in isolation. Do the
same thing here as well.

Drop trivial function which wraps a dropin that also uses $ret, since
it's no longer needed.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---
 scripts/libmakepkg/executable.sh.in          | 2 +-
 scripts/libmakepkg/executable/ccache.sh.in   | 2 +-
 scripts/libmakepkg/executable/distcc.sh.in   | 2 +-
 scripts/libmakepkg/executable/fakeroot.sh.in | 2 +-
 scripts/libmakepkg/executable/gpg.sh.in      | 4 ++++
 scripts/libmakepkg/executable/gzip.sh.in     | 2 +-
 scripts/libmakepkg/executable/pacman.sh.in   | 2 +-
 scripts/libmakepkg/executable/strip.sh.in    | 2 +-
 scripts/libmakepkg/executable/vcs.sh.in      | 8 +-------
 9 files changed, 12 insertions(+), 14 deletions(-)

Comments

Allan McRae Dec. 4, 2018, 7:23 a.m. UTC | #1
On 28/11/18 2:00 pm, Eli Schwartz wrote:
> Elsewhere, we return 1 if a library dropin fails, and when running
> functions in a loop, we use `|| ret=1` to preserve scope. This ensures
> the return value of the function remains useful in isolation. Do the
> same thing here as well.
> 
> Drop trivial function which wraps a dropin that also uses $ret, since
> it's no longer needed.
> 

Thanks - I keep forgetting that the point of libmakepkg is reuseability
in addition to splitting up the massive script...

Allan

Patch

diff --git a/scripts/libmakepkg/executable.sh.in b/scripts/libmakepkg/executable.sh.in
index 68c48038..92031d43 100644
--- a/scripts/libmakepkg/executable.sh.in
+++ b/scripts/libmakepkg/executable.sh.in
@@ -38,7 +38,7 @@  check_software() {
 	local ret=0
 
 	for func in ${executable_functions[@]}; do
-		$func
+		$func || ret=1
 	done
 
 	return $ret
diff --git a/scripts/libmakepkg/executable/ccache.sh.in b/scripts/libmakepkg/executable/ccache.sh.in
index dafde076..6143fee2 100644
--- a/scripts/libmakepkg/executable/ccache.sh.in
+++ b/scripts/libmakepkg/executable/ccache.sh.in
@@ -31,7 +31,7 @@  executable_ccache() {
 	if check_buildoption "ccache" "y"; then
 		if ! type -p ccache >/dev/null; then
 			error "$(gettext "Cannot find the %s binary required for compiler cache usage.")" "ccache"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/distcc.sh.in b/scripts/libmakepkg/executable/distcc.sh.in
index b9883f6b..d3a8cc25 100644
--- a/scripts/libmakepkg/executable/distcc.sh.in
+++ b/scripts/libmakepkg/executable/distcc.sh.in
@@ -31,7 +31,7 @@  executable_distcc() {
 	if check_buildoption "distcc" "y"; then
 		if ! type -p distcc >/dev/null; then
 			error "$(gettext "Cannot find the %s binary required for distributed compilation.")" "distcc"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/fakeroot.sh.in b/scripts/libmakepkg/executable/fakeroot.sh.in
index e22d9a96..56c1b3fd 100644
--- a/scripts/libmakepkg/executable/fakeroot.sh.in
+++ b/scripts/libmakepkg/executable/fakeroot.sh.in
@@ -31,7 +31,7 @@  executable_fakeroot() {
 	if check_buildenv "fakeroot" "y" && (( EUID > 0 )); then
 		if ! type -p fakeroot >/dev/null; then
 			error "$(gettext "Cannot find the %s binary.")" "fakeroot"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/gpg.sh.in b/scripts/libmakepkg/executable/gpg.sh.in
index c0f57013..139773ef 100644
--- a/scripts/libmakepkg/executable/gpg.sh.in
+++ b/scripts/libmakepkg/executable/gpg.sh.in
@@ -28,6 +28,8 @@  source "$LIBRARY/util/option.sh"
 executable_functions+=('executable_gpg')
 
 executable_gpg() {
+	local ret=0
+
 	if [[ $SIGNPKG == 'y' ]] || { [[ -z $SIGNPKG ]] && check_buildenv "sign" "y"; }; then
 		if ! type -p gpg >/dev/null; then
 			error "$(gettext "Cannot find the %s binary required for signing packages.")" "gpg"
@@ -41,4 +43,6 @@  executable_gpg() {
 			ret=1
 		fi
 	fi
+
+	return $ret
 }
diff --git a/scripts/libmakepkg/executable/gzip.sh.in b/scripts/libmakepkg/executable/gzip.sh.in
index 66748320..bb1626bc 100644
--- a/scripts/libmakepkg/executable/gzip.sh.in
+++ b/scripts/libmakepkg/executable/gzip.sh.in
@@ -31,7 +31,7 @@  executable_gzip() {
 	if check_option "zipman" "y"; then
 		if ! type -p gzip >/dev/null; then
 			error "$(gettext "Cannot find the %s binary required for compressing man and info pages.")" "gzip"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/pacman.sh.in b/scripts/libmakepkg/executable/pacman.sh.in
index d9967f45..d1433ffd 100644
--- a/scripts/libmakepkg/executable/pacman.sh.in
+++ b/scripts/libmakepkg/executable/pacman.sh.in
@@ -31,7 +31,7 @@  executable_pacman() {
     if (( ! NODEPS || DEP_BIN || RMDEPS || INSTALL )); then
 		if [[ -z $PACMAN_PATH ]]; then
 			error "$(gettext "Cannot find the %s binary required for dependency operations.")" "$PACMAN"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/strip.sh.in b/scripts/libmakepkg/executable/strip.sh.in
index a6b81db6..867dc928 100644
--- a/scripts/libmakepkg/executable/strip.sh.in
+++ b/scripts/libmakepkg/executable/strip.sh.in
@@ -31,7 +31,7 @@  executable_strip() {
 	if check_option "strip" "y"; then
 		if ! type -p strip >/dev/null; then
 			error "$(gettext "Cannot find the %s binary required for object file stripping.")" "strip"
-			ret=1
+			return 1
 		fi
 	fi
 }
diff --git a/scripts/libmakepkg/executable/vcs.sh.in b/scripts/libmakepkg/executable/vcs.sh.in
index 527d6f22..46631f39 100644
--- a/scripts/libmakepkg/executable/vcs.sh.in
+++ b/scripts/libmakepkg/executable/vcs.sh.in
@@ -49,7 +49,7 @@  get_vcsclient() {
 	printf "%s\n" "$client"
 }
 
-check_vcs_software() {
+executable_vcs() {
 	local netfile all_sources all_deps deps ret=0
 
 	if (( SOURCEONLY == 1 )); then
@@ -101,9 +101,3 @@  check_vcs_software() {
 
 	return $ret
 }
-
-executable_vcs() {
-	if ! check_vcs_software; then
-		ret=1
-	fi
-}