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