[pacman-dev,v2,1/2] makepkg: implement error codes

From: Ivy Foster <ivy.foster@gmail.com>

For your convenience, makepkg now has 16 distinct ways to fail.
---
 doc/makepkg.8.txt                   |  60 ++++++++++++++++++
 scripts/Makefile.am                 |   1 +
 scripts/libmakepkg/util/error.sh.in |  42 +++++++++++++
 scripts/makepkg.sh.in               | 120 ++++++++++++++++++------------------
 4 files changed, 162 insertions(+), 61 deletions(-)
 create mode 100644 scripts/libmakepkg/util/error.sh.in

I didn't go over this in detail, but I'll point out a few concerns I
have about this patch...

On Fri, Sep 15, 2017 at 01:30:51PM -0500, ivy.foster@gmail.com wrote:
> From: Ivy Foster <ivy.foster@gmail.com>
> 
> For your convenience, makepkg now has 16 distinct ways to fail.
> ---
>  doc/makepkg.8.txt                   |  60 ++++++++++++++++++
>  scripts/Makefile.am                 |   1 +
>  scripts/libmakepkg/util/error.sh.in |  42 +++++++++++++
>  scripts/makepkg.sh.in               | 120 ++++++++++++++++++------------------
>  4 files changed, 162 insertions(+), 61 deletions(-)
>  create mode 100644 scripts/libmakepkg/util/error.sh.in
> 
> diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
> index 2dff1b19..224292a2 100644
> --- a/doc/makepkg.8.txt
> +++ b/doc/makepkg.8.txt
> @@ -272,6 +272,66 @@ See linkman:makepkg.conf[5] for more details on configuring makepkg using the
>  'makepkg.conf' file.
>  
>  
> +Errors
> +------
> +On exit, makepkg will return one of the following error codes.
> +
> +**E_OK**=0::

I don't see the need to document internal details of how we refer to the
error codes through named variables if we aren't making these public API.

> +	Normal exit condition.
> +
> +**E_FAIL**=1::
> +	Unknown cause of failure.
> +
> +// Exit code 2 is reserved by bash for misuse of shell builtins
> +
> +**E_CONFIG_ERROR**=3::
> +	Error in configuration file.
> +
> +**E_INVALID_OPTION**=4::
> +	User specified an invalid option
> +
> +**E_BUILD_FAILED**=5::
> +	Error in PKGBUILD build function.
> +
> +**E_PACKAGE_FAILED**=6::
> +	Failed to create a viable package.

What about failures in the prepare of pkgver functions (and any future
functions which haven't yet been defined)? I think this ought to be more
generic and be something like E_USER_FUNCTION_FAILED.

> +**E_MISSING_FILE**=7::
> +	A source or auxiliary file specified in the PKGBUILD is
> +	missing.
> +
> +**E_MISSING_PKGDIR**=8::
> +	The PKGDIR is missing.
> +
> +**E_INSTALL_DEPS_FAILED**=9::
> +	Failed to install dependencies.
> +
> +**E_REMOVE_DEPS_FAILED**=10::
> +	Failed to remove dependencies.
> +
> +**E_ROOT**=11::
> +	User attempted to run makepkg as root.
> +
> +**E_FS_PERMISSIONS**=12::
> +	User lacks permissions to build or install to a given
> +	location.
> +
> +**E_PKGBUILD_ERROR**=13::
> +	Error parsing PKGBUILD.
> +
> +**E_ALREADY_BUILT**=14::
> +	A package has already been built.
> +
> +**E_INSTALL_FAILED**=15::
> +	The package failed to install.
> +
> +**E_MISSING_MAKEPKG_DEPS**=16::
> +	Programs necessary to run makepkg are missing.
> +
> +**E_PRETTY_BAD_PRIVACY**=17::
> +	Error signing package.

As implemented, this is only used when checking to see that the key
exists, not as a failure when signing the package. To do that, you'd
need to change scripts/libmakepkg/integrity/generate_signature.sh.in,
and then make sure the error code is propagated down the stack.

> +
> +
>  See Also
>  --------
>  linkman:makepkg.conf[5], linkman:PKGBUILD[5], linkman:pacman[8]
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 4bb08a24..3e7689bf 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -96,6 +96,7 @@ LIBMAKEPKG_IN = \
>  	libmakepkg/tidy/strip.sh \
>  	libmakepkg/tidy/zipman.sh \
>  	libmakepkg/util.sh \
> +	libmakepkg/util/error.sh \
>  	libmakepkg/util/message.sh \
>  	libmakepkg/util/option.sh \
>  	libmakepkg/util/parseopts.sh \
> diff --git a/scripts/libmakepkg/util/error.sh.in b/scripts/libmakepkg/util/error.sh.in
> new file mode 100644
> index 00000000..eefd5652
> --- /dev/null
> +++ b/scripts/libmakepkg/util/error.sh.in
> @@ -0,0 +1,42 @@
> +#!/bin/bash
> +#
> +#   error.sh.in - error variable definitions for makepkg
> +#
> +#   Copyright (c) 2006-2017 Pacman Development Team <pacman-dev@archlinux.org>
> +#   Copyright (c) 2002-2006 by Judd Vinet <jvinet@zeroflux.org>
> +#
> +#   This program is free software; you can redistribute it and/or modify
> +#   it under the terms of the GNU General Public License as published by
> +#   the Free Software Foundation; either version 2 of the License, or
> +#   (at your option) any later version.
> +#
> +#   This program is distributed in the hope that it will be useful,
> +#   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#   GNU General Public License for more details.
> +#
> +#   You should have received a copy of the GNU General Public License
> +#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +[[ -n "$LIBMAKEPKG_UTIL_ERROR_SH" ]] && return
> +LIBMAKEPKG_UTIL_ERROR_SH=1
> +
> +E_OK=0
> +E_FAIL=1 # Generic error
> +# exit code 2 reserved by bash for misuse of shell builtins

Not sure I understand this... When would this clash?

> +E_CONFIG_ERROR=3
> +E_INVALID_OPTION=4
> +E_BUILD_FAILED=5
> +E_PACKAGE_FAILED=6
> +E_MISSING_FILE=7
> +E_MISSING_PKGDIR=8
> +E_INSTALL_DEPS_FAILED=9
> +E_REMOVE_DEPS_FAILED=10
> +E_ROOT=11
> +E_FS_PERMISSIONS=12
> +E_PKGBUILD_ERROR=13
> +E_ALREADY_BUILT=14
> +E_INSTALL_FAILED=15
> +E_MISSING_MAKEPKG_DEPS=16
> +E_PRETTY_BAD_PRIVACY=17
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 20e9dd7e..a8a8eb41 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -130,7 +130,7 @@ clean_up() {
>  		return
>  	fi
>  
> -	if (( ! EXIT_CODE && CLEANUP )); then
> +	if (( (EXIT_CODE == E_OK || EXIT_CODE == E_INSTALL_FAILED) && CLEANUP )); then
>  		local pkg file
>  
>  		# If it's a clean exit and -c/--clean has been passed...
> @@ -184,7 +184,7 @@ update_pkgver() {
>  	newpkgver=$(run_function_safe pkgver)
>  	if ! check_pkgver "$newpkgver"; then
>  		error "$(gettext "pkgver() generated an invalid version: %s")" "$newpkgver"
> -		exit 1
> +		exit $E_PKGBUILD_ERROR
>  	fi
>  
>  	if [[ -n $newpkgver && $newpkgver != "$pkgver" ]]; then
> @@ -192,7 +192,7 @@ update_pkgver() {
>  			if ! @SEDINPLACE@ "s:^pkgver=[^ ]*:pkgver=$newpkgver:" "$BUILDFILE"; then
>  				error "$(gettext "Failed to update %s from %s to %s")" \
>  						"pkgver" "$pkgver" "$newpkgver"
> -				exit 1
> +				exit $E_PKGBUILD_ERROR
>  			fi
>  			@SEDINPLACE@ "s:^pkgrel=[^ ]*:pkgrel=1:" "$BUILDFILE"
>  			source_safe "$BUILDFILE"
> @@ -209,7 +209,7 @@ update_pkgver() {
>  missing_source_file() {
>  	error "$(gettext "Unable to find source file %s.")" "$(get_filename "$1")"
>  	plain "$(gettext "Aborting...")"
> -	exit 1 # $E_MISSING_FILE
> +	exit $E_MISSING_FILE
>  }
>  
>  run_pacman() {
> @@ -263,7 +263,7 @@ handle_deps() {
>  
>  		if ! run_pacman -S --asdeps "${deplist[@]}"; then
>  			error "$(gettext "'%s' failed to install missing dependencies.")" "$PACMAN"
> -			exit 1 # TODO: error code
> +			exit $E_INSTALL_DEPS_FAILED
>  		fi
>  	fi
>  
> @@ -284,12 +284,12 @@ resolve_deps() {
>  	# deplist cannot be declared like this: local deplist=$(foo)
>  	# Otherwise, the return value will depend on the assignment.
>  	local deplist
> -	deplist=($(check_deps "$@")) || exit 1
> +	deplist=($(check_deps "$@")) || exit $E_INSTALL_DEPS_FAILED
>  	[[ -z $deplist ]] && return $R_DEPS_SATISFIED
>  
>  	if handle_deps "${deplist[@]}"; then
>  		# check deps again to make sure they were resolved
> -		deplist=$(check_deps "$@") || exit 1
> +		deplist=$(check_deps "$@") || exit $E_INSTALL_DEPS_FAILED
>  		[[ -z $deplist ]] && return $R_DEPS_SATISFIED
>  	fi
>  
> @@ -310,7 +310,7 @@ remove_deps() {
>  	if [[ -n $(grep -xvFf <(printf '%s\n' "${current_pkglist[@]}") \
>  			<(printf '%s\n' "${original_pkglist[@]}")) ]]; then
>  		warning "$(gettext "Failed to remove installed dependencies.")"
> -		return 0
> +		return $E_REMOVE_DEPS_FAILED
>  	fi
>  
>  	local deplist
> @@ -324,7 +324,7 @@ remove_deps() {
>  	# exit cleanly on failure to remove deps as package has been built successfully
>  	if ! run_pacman -Rn ${deplist[@]}; then
>  		warning "$(gettext "Failed to remove installed dependencies.")"
> -		return 0
> +		return $E_REMOVE_DEPS_FAILED
>  	fi
>  }
>  
> @@ -337,14 +337,14 @@ error_function() {
>  		error "$(gettext "A failure occurred in %s().")" "$1"
>  		plain "$(gettext "Aborting...")"
>  	fi
> -	exit 2 # $E_BUILD_FAILED
> +	exit $E_BUILD_FAILED
>  }
>  
>  source_safe() {
>  	shopt -u extglob
>  	if ! source "$@"; then
>  		error "$(gettext "Failed to source %s")" "$1"
> -		exit 1
> +		exit $E_MISSING_FILE
>  	fi
>  	shopt -s extglob
>  }
> @@ -615,7 +615,7 @@ write_kv_pair() {
>  	for val in "$@"; do
>  		if [[ $val = *$'\n'* ]]; then
>  			error "$(gettext "Invalid value for %s: %s")" "$key" "$val"
> -			exit 1
> +			exit $E_PKGBUILD_ERROR
>  		fi
>  		printf "%s = %s\n" "$key" "$val"
>  	done
> @@ -704,7 +704,7 @@ create_package() {
>  	if [[ ! -d $pkgdir ]]; then
>  		error "$(gettext "Missing %s directory.")" "\$pkgdir/"
>  		plain "$(gettext "Aborting...")"
> -		exit 1 # $E_MISSING_PKGDIR
> +		exit $E_MISSING_PKGDIR
>  	fi
>  
>  	cd_safe "$pkgdir"
> @@ -722,7 +722,7 @@ create_package() {
>  			msg2 "$(gettext "Adding %s file...")" "$orig"
>  			if ! cp "$startdir/${!orig}" "$dest"; then
>  				error "$(gettext "Failed to add %s file to package.")" "$orig"
> -				exit 1
> +				exit $E_MISSING_FILE
>  			fi
>  			chmod 644 "$dest"
>  		fi
> @@ -768,7 +768,7 @@ create_package() {
>  
>  	if (( ret )); then
>  		error "$(gettext "Failed to create package file.")"
> -		exit 1 # TODO: error code
> +		exit $E_PACKAGE_FAILED
>  	fi
>  }
>  
> @@ -864,14 +864,13 @@ create_srcpackage() {
>  	cd_safe "${srclinks}"
>  	if ! LANG=C bsdtar -cL ${TAR_OPT} -f "$pkg_file" ${pkgbase}; then
>  		error "$(gettext "Failed to create source package file.")"
> -		exit 1 # TODO: error code
> +		exit $E_PACKAGE_FAILED
>  	fi
>  
>  	cd_safe "${startdir}"
>  	rm -rf "${srclinks}"
>  }
>  
> -# this function always returns 0 to make sure clean-up will still occur
>  install_package() {
>  	(( ! INSTALL )) && return
>  
> @@ -897,7 +896,7 @@ install_package() {
>  
>  	if ! run_pacman -U "${pkglist[@]}"; then
>  		warning "$(gettext "Failed to install built package(s).")"
> -		return 0
> +		return $E_INSTALL_FAILED
>  	fi
>  }
>  
> @@ -917,7 +916,7 @@ get_vcsclient() {
>  	if [[ -z $client ]]; then
>  		error "$(gettext "Unknown download protocol: %s")" "$proto"
>  		plain "$(gettext "Aborting...")"
> -		exit 1 # $E_CONFIG_ERROR
> +		exit $E_CONFIG_ERROR
>  	fi
>  
>  	printf "%s\n" "$client"
> @@ -956,7 +955,7 @@ check_vcs_software() {
>  					client=$(get_vcsclient "$proto") || exit $?
>  					# ensure specified program is installed
>  					local uninstalled
> -					uninstalled=$(check_deps "$client") || exit 1
> +					uninstalled=$(check_deps "$client") || exit $E_INSTALL_DEPS_FAILED
>  					# if not installed, check presence in depends or makedepends
>  					if [[ -n "$uninstalled" ]] && (( ! NODEPS || ( VERIFYSOURCE && !DEP_BIN ) )); then
>  						if ! in_array "$client" ${all_deps[@]}; then
> @@ -1081,11 +1080,11 @@ check_build_status() {
>  				 && ! (( FORCE || SOURCEONLY || NOBUILD || NOARCHIVE)); then
>  			if (( INSTALL )); then
>  				warning "$(gettext "A package has already been built, installing existing package...")"
> -				install_package
> -				exit 0
> +				status=$(install_package)
> +				exit $status
>  			else
>  				error "$(gettext "A package has already been built. (use %s to overwrite)")" "-f"
> -				exit 1
> +				exit $E_ALREADY_BUILT
>  			fi
>  		fi
>  	else
> @@ -1104,16 +1103,16 @@ check_build_status() {
>  			if (( allpkgbuilt )); then
>  				if (( INSTALL )); then
>  					warning "$(gettext "The package group has already been built, installing existing packages...")"
> -					install_package
> -					exit 0
> +					status=$(install_package)
> +					exit $status
>  				else
>  					error "$(gettext "The package group has already been built. (use %s to overwrite)")" "-f"
> -					exit 1
> +					exit $E_ALREADY_BUILT
>  				fi
>  			fi
>  			if (( somepkgbuilt && ! PKGVERFUNC )); then
>  				error "$(gettext "Part of the package group has already been built. (use %s to overwrite)")" "-f"
> -				exit 1
> +				exit $E_ALREADY_BUILT
>  			fi
>  		fi
>  		unset allpkgbuilt somepkgbuilt
> @@ -1148,7 +1147,7 @@ run_split_packaging() {
>  		backup_package_variables
>  		run_package $pkgname
>  		tidy_install
> -		lint_package || exit 1
> +		lint_package || exit $E_PACKAGE_FAILED
>  		create_package
>  		restore_package_variables
>  	done
> @@ -1245,7 +1244,7 @@ OPT_LONG=('allsource' 'check' 'clean' 'cleanbuild' 'config:' 'force' 'geninteg'
>  OPT_LONG+=('asdeps' 'noconfirm' 'needed' 'noprogressbar')
>  
>  if ! parseopts "$OPT_SHORT" "${OPT_LONG[@]}" -- "$@"; then
> -	exit 1 # E_INVALID_OPTION;
> +	exit $E_INVALID_OPTION;
>  fi
>  set -- "${OPTRET[@]}"
>  unset OPT_SHORT OPT_LONG OPTRET
> @@ -1294,8 +1293,8 @@ while true; do
>  		-S|--source)      SOURCEONLY=1 ;;
>  		--verifysource)   VERIFYSOURCE=1 ;;
>  
> -		-h|--help)        usage; exit 0 ;; # E_OK
> -		-V|--version)     version; exit 0 ;; # E_OK
> +		-h|--help)        usage; exit $E_OK ;;
> +		-V|--version)     version; exit $E_OK ;;
>  
>  		--)               OPT_IND=0; shift; break 2;;
>  	esac
> @@ -1341,7 +1340,7 @@ if [[ -r $MAKEPKG_CONF ]]; then
>  else
>  	error "$(gettext "%s not found.")" "$MAKEPKG_CONF"
>  	plain "$(gettext "Aborting...")"
> -	exit 1 # $E_CONFIG_ERROR
> +	exit $E_CONFIG_ERROR
>  fi
>  
>  # Source user-specific makepkg.conf overrides, but only if no override config
> @@ -1375,14 +1374,14 @@ if [[ ! -d $BUILDDIR ]]; then
>  	if ! mkdir -p "$BUILDDIR"; then
>  		error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
>  		plain "$(gettext "Aborting...")"
> -		exit 1
> +		exit $E_FS_PERMISSIONS
>  	fi
>  	chmod a-s "$BUILDDIR"
>  fi
>  if [[ ! -w $BUILDDIR ]]; then
>  	error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
>  	plain "$(gettext "Aborting...")"
> -	exit 1
> +	exit $E_FS_PERMISSIONS
>  fi
>  
>  # override settings from extra variables on commandline, if any
> @@ -1395,7 +1394,7 @@ PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined
>  if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then
>  	error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST"
>  	plain "$(gettext "Aborting...")"
> -	exit 1
> +	exit $E_FS_PERMISSIONS
>  fi
>  
>  SRCDEST=${_SRCDEST:-$SRCDEST}
> @@ -1403,7 +1402,7 @@ SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined
>  if [[ ! -w $SRCDEST ]] ; then
>  	error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST"
>  	plain "$(gettext "Aborting...")"
> -	exit 1
> +	exit $E_FS_PERMISSIONS
>  fi
>  
>  SRCPKGDEST=${_SRCPKGDEST:-$SRCPKGDEST}
> @@ -1412,7 +1411,7 @@ if (( SOURCEONLY )); then
>  	if [[ ! -w $SRCPKGDEST ]]; then
>  		error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST"
>  		plain "$(gettext "Aborting...")"
> -		exit 1
> +		exit $E_FS_PERMISSIONS
>  	fi
>  
>  	# If we're only making a source tarball, then we need to ignore architecture-
> @@ -1425,7 +1424,7 @@ LOGDEST=${LOGDEST:-$startdir} #default to $startdir if undefined
>  if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then
>  	error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST"
>  	plain "$(gettext "Aborting...")"
> -	exit 1
> +	exit $E_FS_PERMISSIONS
>  fi
>  
>  PKGEXT=${_PKGEXT:-$PKGEXT}
> @@ -1439,12 +1438,12 @@ if (( ! INFAKEROOT )); then
>  	if (( EUID == 0 )); then
>  		error "$(gettext "Running %s as root is not allowed as it can cause permanent,\n\
>  catastrophic damage to your system.")" "makepkg"
> -		exit 1 # $E_USER_ABORT
> +		exit $E_ROOT
>  	fi
>  else
>  	if [[ -z $FAKEROOTKEY ]]; then
>  		error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg"
> -		exit 1 # TODO: error code
> +		exit $E_INVALID_OPTION
>  	fi
>  fi
>  
> @@ -1459,16 +1458,17 @@ unset "${!sha384sums_@}" "${!sha512sums_@}"
>  BUILDFILE=${BUILDFILE:-$BUILDSCRIPT}
>  if [[ ! -f $BUILDFILE ]]; then
>  	error "$(gettext "%s does not exist.")" "$BUILDFILE"
> -	exit 1
> +	exit $E_BUILD_FAILED=5
> +
>  else
>  	if [[ $(<"$BUILDFILE") = *$'\r'* ]]; then
>  		error "$(gettext "%s contains %s characters and cannot be sourced.")" "$BUILDFILE" "CRLF"
> -		exit 1
> +		exit $E_PKGBUILD_ERROR
>  	fi
>  
>  	if [[ ! $BUILDFILE -ef $PWD/${BUILDFILE##*/} ]]; then
>  		error "$(gettext "%s must be in the current working directory.")" "$BUILDFILE"
> -		exit 1
> +		exit $E_PKGBUILD_ERROR
>  	fi
>  
>  	if [[ ${BUILDFILE:0:1} != "/" ]]; then
> @@ -1480,7 +1480,7 @@ fi
>  pkgbase=${pkgbase:-${pkgname[0]}}
>  
>  # check the PKGBUILD for some basic requirements
> -lint_pkgbuild || exit 1
> +lint_pkgbuild || exit $E_PKGBUILD_ERROR
>  
>  if (( !SOURCEONLY && !PRINTSRCINFO )); then
>  	merge_arch_attrs
> @@ -1506,7 +1506,7 @@ if (( GENINTEG )); then
>  	cd_safe "$srcdir"
>  	download_sources novcs allarch
>  	generate_checksums
> -	exit 0 # $E_OK
> +	exit $E_OK
>  fi
>  
>  if have_function pkgver; then
> @@ -1514,7 +1514,7 @@ if have_function pkgver; then
>  fi
>  
>  # check we have the software required to process the PKGBUILD
> -check_software || exit 1
> +check_software || exit $E_MISSING_MAKEPKG_DEPS
>  
>  if (( ${#pkgname[@]} > 1 )); then
>  	SPLITPKG=1
> @@ -1551,18 +1551,18 @@ if { [[ -z $SIGNPKG ]] && check_buildenv "sign" "y"; } || [[ $SIGNPKG == 'y' ]];
>  		else
>  			error "$(gettext "There is no key in your keyring.")"
>  		fi
> -		exit 1
> +		exit $E_PRETTY_BAD_PRIVACY
>  	fi
>  fi
>  
>  if (( PACKAGELIST )); then
>  	print_all_package_names
> -	exit 0
> +	exit $E_OK
>  fi
>  
>  if (( PRINTSRCINFO )); then
>  	write_srcinfo_content
> -	exit 0
> +	exit $E_OK
>  fi
>  
>  if (( ! PKGVERFUNC )); then
> @@ -1574,7 +1574,7 @@ if (( INFAKEROOT )); then
>  	if (( SOURCEONLY )); then
>  		create_srcpackage
>  		msg "$(gettext "Leaving %s environment.")" "fakeroot"
> -		exit 0 # $E_OK
> +		exit $E_OK
>  	fi
>  
>  	prepare_buildenv
> @@ -1587,7 +1587,7 @@ if (( INFAKEROOT )); then
>  			run_package
>  		fi
>  		tidy_install
> -		lint_package || exit 1
> +		lint_package || exit $E_PACKAGE_FAILED
>  		create_package
>  		create_debug_package
>  	else
> @@ -1595,7 +1595,7 @@ if (( INFAKEROOT )); then
>  	fi
>  
>  	msg "$(gettext "Leaving %s environment.")" "fakeroot"
> -	exit 0 # $E_OK
> +	exit $E_OK
>  fi
>  
>  msg "$(gettext "Making package: %s")" "$pkgbase $basever ($(date))"
> @@ -1605,7 +1605,7 @@ if (( SOURCEONLY )); then
>  	if [[ -f $SRCPKGDEST/${pkgbase}-${basever}${SRCEXT} ]] \
>  			&& (( ! FORCE )); then
>  		error "$(gettext "A source package has already been built. (use %s to overwrite)")" "-f"
> -		exit 1
> +		exit $E_ALREADY_BUILT
>  	fi
>  
>  	# Get back to our src directory so we can begin with sources.
> @@ -1627,7 +1627,7 @@ if (( SOURCEONLY )); then
>  	create_signature "$SRCPKGDEST/${pkgbase}-${fullver}${SRCEXT}"
>  
>  	msg "$(gettext "Source package created: %s")" "$pkgbase ($(date))"
> -	exit 0
> +	exit $E_OK
>  fi
>  
>  if (( NODEPS || ( VERIFYSOURCE && !DEP_BIN ) )); then
> @@ -1660,7 +1660,7 @@ else
>  
>  	if (( deperr )); then
>  		error "$(gettext "Could not resolve all dependencies.")"
> -		exit 1
> +		exit $E_INSTALL_DEPS_FAILED
>  	fi
>  fi
>  
> @@ -1675,7 +1675,7 @@ if (( !REPKG )); then
>  	else
>  		download_sources
>  		check_source_integrity
> -		(( VERIFYSOURCE )) && exit 0 # $E_OK
> +		(( VERIFYSOURCE )) && exit $E_OK
>  
>  		if (( CLEANBUILD )); then
>  			msg "$(gettext "Removing existing %s directory...")" "\$srcdir/"
> @@ -1697,7 +1697,7 @@ fi
>  
>  if (( NOBUILD )); then
>  	msg "$(gettext "Sources are ready.")"
> -	exit 0 #E_OK
> +	exit $E_OK
>  else
>  	# clean existing pkg directory
>  	if [[ -d $pkgdirbase ]]; then
> @@ -1724,13 +1724,11 @@ fi
>  # if inhibiting archive creation, go no further
>  if (( NOARCHIVE )); then
>  	msg "$(gettext "Package directory is ready.")"
> -	exit 0
> +	exit $E_OK
>  fi
>  
>  msg "$(gettext "Finished making: %s")" "$pkgbase $basever ($(date))"
>  
> -install_package
> -
> -exit 0 #E_OK
> +install_package && exit $E_OK || exit $E_INSTALL_FAILED
>  
>  # vim: set noet:
> -- 
> 2.14.1

Dave Reisner <d@falconindy.com> wrote:
> I didn't go over this in detail, but I'll point out a few concerns I
> have about this patch...

Fair enough, thanks for the feedback.

> On Fri, Sep 15, 2017 at 01:30:51PM -0500, ivy.foster@gmail.com wrote:
> > +Errors
> > +------
> > +On exit, makepkg will return one of the following error codes.
> > +
> > +**E_OK**=0::
> 
> I don't see the need to document internal details of how we refer to the
> error codes through named variables if we aren't making these public API.

The rationale here was that it could be useful information for anybody
scripting builds, but I don't feel strongly about it. I do see your
point; anybody using these to (say) make tests for makepkg can easily
figure out what they all mean from the names in the source.

> > +**E_BUILD_FAILED**=5::
> > +   Error in PKGBUILD build function.
> > +
> > +**E_PACKAGE_FAILED**=6::
> > +   Failed to create a viable package.
> 
> What about failures in the prepare of pkgver functions (and any future
> functions which haven't yet been defined)? I think this ought to be more
> generic and be something like E_USER_FUNCTION_FAILED.

That's a good idea.

> > +**E_PRETTY_BAD_PRIVACY**=17::
> > +   Error signing package.
> 
> As implemented, this is only used when checking to see that the key
> exists, not as a failure when signing the package. To do that, you'd
> need to change scripts/libmakepkg/integrity/generate_signature.sh.in,
> and then make sure the error code is propagated down the stack.

...you're quite right. Will fix.

> > +# exit code 2 reserved by bash for misuse of shell builtins
> 
> Not sure I understand this... When would this clash?

I'm not sure that it would; I just happened across that tidbit in
tldp's bash scripting guide while looking for something else. Further
research (actually looking it up in bash(1)) reveals that it isn't
actually *reserved*, just used for that by bash. Will fix.

Thanks again for the critique. I'll get this stuff cleaned up sometime
in the next couple of days.

iff

On 16/09/17 06:54, Dave Reisner wrote:
>> +Errors
>> +------
>> +On exit, makepkg will return one of the following error codes.
>> +
>> +**E_OK**=0::
> I don't see the need to document internal details of how we refer to the
> error codes through named variables if we aren't making these public API.
> 

To clarify - you are saying to remove "E_OK" etc from the documentation,
but keep the number and description? That seems fair enough to me.

Allan

On Sun, Sep 17, 2017 at 05:34:15PM +1000, Allan McRae wrote:
> On 16/09/17 06:54, Dave Reisner wrote:
> >> +Errors
> >> +------
> >> +On exit, makepkg will return one of the following error codes.
> >> +
> >> +**E_OK**=0::
> > I don't see the need to document internal details of how we refer to the
> > error codes through named variables if we aren't making these public API.
> > 
> 
> To clarify - you are saying to remove "E_OK" etc from the documentation,
> but keep the number and description? That seems fair enough to me.
> 
> Allan

Yep, that's correct. Similar to how curl(1) documents its exit codes.

Dave Reisner <d@falconindy.com> wrote:
> On Sun, Sep 17, 2017 at 05:34:15PM +1000, Allan McRae wrote:
> > On 16/09/17 06:54, Dave Reisner wrote:
> >>> +Errors
> >>> +------
> >>> +On exit, makepkg will return one of the following error codes.
> >>> +
> >>> +**E_OK**=0::
> >> I don't see the need to document internal details of how we refer to the
> >> error codes through named variables if we aren't making these public API.
> > 
> > To clarify - you are saying to remove "E_OK" etc from the documentation,
> > but keep the number and description? That seems fair enough to me.

> Yep, that's correct. Similar to how curl(1) documents its exit codes.

Ah, yes, I see what you mean.

iff

Am 15.09.2017 um 20:30 schrieb ivy.foster@gmail.com:
> @@ -1459,16 +1458,17 @@ unset "${!sha384sums_@}" "${!sha512sums_@}"
>  BUILDFILE=${BUILDFILE:-$BUILDSCRIPT}
>  if [[ ! -f $BUILDFILE ]]; then
>  	error "$(gettext "%s does not exist.")" "$BUILDFILE"
> -	exit 1
> +	exit $E_BUILD_FAILED=5

Small thing everyone seems to have overlooked:
This line looks wrong to me.
bash probably won't like it either. ;)

This attempt fixes the error, pointed out by brainpower, in which I
accidentally dropped in an assignment when I was trying to use one of
the error variables.

This time, this patch depends on the trivial wording patch sent
earlier today, rather than the other way around.

It also incorporates Dave's suggestions:
- The manpage no longer names the errors.
- E_PRETTY_BAD_PRIVACY is correctly documented (its scope could
  probably stand to be expanded to include actual GPG errors, but
  that's another patch).
- E_BUILD_FAILED has been renamed E_USER_FUNCTION_FAILED, though I
  kept E_PACKAGE_FAILED because as implemented, it's being used for
  exits on the packaging process as a whole, rather than for the user
  package function.
- Error code 2 is no longer skipped over, since bash doesn't actually
  *reserve* it.

Thanks to everyone for the feedback!

iff