[pacman-dev] RFC: makepkg errors
I've implemented a patch that allows makepkg to return discrete error values for different errors. This is based on the comments scattered throughout indicating that at some point, somebody intended to implement this. As a side benefit, it also closes [FS#54204][], since being able to return a recognizable error from a failed install_package means that we can still do clean-up even if this function fails. Cons: That's a lot of different types of error. As always, I welcome your critiques! iff Links: [FS#54204]: https://bugs.archlinux.org/task/54204
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:: + 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. + +**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. + + 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 +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
From: Ivy Foster <ivy.foster@gmail.com> --- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index a8a8eb41..953c43fb 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1442,7 +1442,7 @@ catastrophic damage to your system.")" "makepkg" fi else if [[ -z $FAKEROOTKEY ]]; then - error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg" + error "$(gettext "Do not use the %s option. This option is only for internal use by %s.")" "'-F'" "makepkg" exit $E_INVALID_OPTION fi fi -- 2.14.1
On 15.09.2017 20:30, ivy.foster@gmail.com wrote:
- error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg" + error "$(gettext "Do not use the %s option. This option is only for internal use by %s.")" "'-F'" "makepkg"
This seems like a very straight forward change. It's generally better if you commit less invasive/more likely to be merged changes first so that they can be merged more easily. Right now this patch should raise a merge conflict if merged without the first one because of the changed exit line. You also won't have to carry it along each time you make a change to the more complex patch. In case you've never done this before: Committing like this can be done rather easily if you have all the changes in your working directory and then use `git add -pi`. When it asks you to stage the hunk, you can edit the diff and take out the exit code change and then stage only the error message. Then commit and make a second commit for the rest. Obviously you could also just commit it afterwards, perform an interactive rebase to reorder the history and then fix the conflicts, but I generally follow the above approach when I have small fixes that come up during creation of a larger patch. It also ensures that `git diff` is usable during development and I don't have to track the possibility of performing that change afterwards so I can't forget it. Also what's done is done. Florian
Florian Pritz via pacman-dev <pacman-dev@archlinux.org> wrote:
On 15.09.2017 20:30, ivy.foster@gmail.com wrote:
- error "$(gettext "Do not use the %s option. \ This option is only for use by %s.")" "'-F'" "makepkg" + error "$(gettext "Do not use the %s option. \ This option is only for internal use by %s.")" "'-F'" "makepkg"
This seems like a very straight forward change. It's generally better if you commit less invasive/more likely to be merged changes first so that they can be merged more easily.
That makes a lot of sense.
In case you've never done this before: [snip]
That's very handy! Thanks, Florian. iff
Florian Pritz via pacman-dev <pacman-dev@archlinux.org> wrote:
This seems like a very straight forward change. It's generally better if you commit less invasive/more likely to be merged changes first so that they can be merged more easily.
I've just sent a version of this patch that does not depend on the makepkg errors patch. Revised makepkg errors patch to follow. iff
From: Ivy Foster <ivy.foster@gmail.com> --- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 20e9dd7e..46eefd1f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1443,7 +1443,7 @@ catastrophic damage to your system.")" "makepkg" fi else if [[ -z $FAKEROOTKEY ]]; then - error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg" + error "$(gettext "Do not use the %s option. This option is only for internal use by %s.")" "'-F'" "makepkg" exit 1 # TODO: error code fi fi -- 2.14.1
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. ;) -- regards, brainpower
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
From: Ivy Foster <ivy.foster@gmail.com> For your convenience, makepkg now has 16 distinct ways to fail. Also closes FS#54204. --- doc/makepkg.8.txt | 57 +++++++++++++++++ scripts/Makefile.am | 1 + scripts/libmakepkg/util/error.sh.in | 41 ++++++++++++ scripts/makepkg.sh.in | 120 ++++++++++++++++++------------------ 4 files changed, 158 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..37d4b7ae 100644 --- a/doc/makepkg.8.txt +++ b/doc/makepkg.8.txt @@ -272,6 +272,63 @@ 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. + +0:: + Normal exit condition. + +1:: + Unknown cause of failure. + +2:: + Error in configuration file. + +3:: + User specified an invalid option + +4:: + Error in user-supplied function in PKGBUILD. + +5:: + Failed to create a viable package. + +6:: + A source or auxiliary file specified in the PKGBUILD is + missing. + +7:: + The PKGDIR is missing. + +8:: + Failed to install dependencies. + +9:: + Failed to remove dependencies. + +10:: + User attempted to run makepkg as root. + +11:: + User lacks permissions to build or install to a given + location. + +12:: + Error parsing PKGBUILD. + +13:: + A package has already been built. + +14:: + The package failed to install. + +15:: + Programs necessary to run makepkg are missing. + +16:: + Specified GPG key does not exist. + 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..1ddc214d --- /dev/null +++ b/scripts/libmakepkg/util/error.sh.in @@ -0,0 +1,41 @@ +#!/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 +E_CONFIG_ERROR=2 +E_INVALID_OPTION=3 +E_USER_FUNCTION_FAILED=4 +E_PACKAGE_FAILED=5 +E_MISSING_FILE=6 +E_MISSING_PKGDIR=7 +E_INSTALL_DEPS_FAILED=8 +E_REMOVE_DEPS_FAILED=9 +E_ROOT=10 +E_FS_PERMISSIONS=11 +E_PKGBUILD_ERROR=12 +E_ALREADY_BUILT=13 +E_INSTALL_FAILED=14 +E_MISSING_MAKEPKG_DEPS=15 +E_PRETTY_BAD_PRIVACY=16 diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 46eefd1f..3b9a54e0 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_USER_FUNCTION_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 internal 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_USER_FUNCTION_FAILED + 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
participants (6)
-
Allan McRae
-
brainpower
-
Dave Reisner
-
Florian Pritz
-
Ivy Foster
-
ivy.foster＠gmail.com