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.fos...@gmail.com wrote: > From: Ivy Foster <ivy.fos...@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 <jvi...@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