commit 1888bd198b938a8d2cbb109ba9cf1bb6c09b6e6f Author: Gregory M. Turner Date: Mon Oct 22 17:47:50 2012 -0700 Fix bugs in __helpers_die() and nonfatal This implements the following changes: bin/isolated-functions.sh: __helpers_die(): rewire to only use die() when actually die()ing; use annotated ewarn instead of unceremoneously dumping to stderr when deciding not to die after all; memorize currently-active error return code upon invocation and return with this code when not die()ing; document this functionality. bin/isolated-functions.sh: die(): remove code preventing death from actually occuring when [[ ${PORTAGE_NONFATAL} -eq 1 ]] bin/isolated-functions.sh: __assert_sigpipe_ok(): use __helpers_die instead of die; also, rename to __helpers_assert_sigpipe_ok; reformat $foo variable syntax to ${foo} for consistency bin/isolated-functions.sh: nonfatal: don't die when nonfatal command is issued without arguments; instead emit an error and return 1 bin/phase-helpers.sh: unpack(): use __helpers_die() instead of die() & __helpers_assert_sigpipe_ok() instead of __assert_sigpipe_ok(); reformat $foo variable syntax to ${foo} for consistency; eliminate created_symlink variable and instead just conditionalize the entire block of code it affected (this small noop change seemed easier to understand); rewrite 'find | xargs' as 'find -execdir' when adjusting permissions so that we can handle errors appropriately without __helpers_assert_sigpipe_ok. bin/phase-helpers.sh: econf(): paranoia: quote "$1" in __hasg() subfunction; use __helpers_die instead of die(). bin/phase-helpers.sh: unpack_tar(): use the new name __helpers_assert_sigpipe_ok instead of __assert_sigpipe_ok bin/phase-helpers.sh: unpack(): likewise bin/save-ebuild-env.sh: __save_ebuild_env(): likewise doc/package/ebuild/eapi/4.docbook: Clarify some things about how nonfatal, die, assert, and helper functions relate to each other. In particular, make it clear that nonfatal is not designed to be applied to constructs other than helper function invocations. Give an example of how to use nonfatal. Document explicitly that the die() and assert() helpers are not affected by nonfatal. These changes are an attempt to address the issues identified in this thread: http://thread.gmane.org/gmane.linux.gentoo.devel/80780 In addition to the issues and solutions agreed upon in that thread, the patch makes one additional "change": it allows assert() to continue to die() regardless of PORTAGE_NONFATAL. Since assert() was already calling die(), no change to the source was neccesary to achieve this; but it is worth noting, because this is nevertheless a novel semantic change I have decided to make without consulting anybody. I am pretty darn confident that it's the right change to make, given the consensus from the aformentioned thread, however, I suppose I'm not 100% sure and some SME should approve this change -- it may require some kind of update to the PMS. (a bit off topic: IMHO, the PMS content pertaining to this entire matter could really use some editing, but I'll leave that to others to fix, since I don't very well understand the process by which PMS changes are made. If someone would like to clue me in on that, drop me a line at the address to be found at http://gmturner.com/contact; I'd be happy to take a crack at it, now that I feel like I understand the intent behind the existing language.) Signed-off-by: Gregory M. Turner diff --git a/bin/isolated-functions.sh b/bin/isolated-functions.sh index f8e7862..16f084c 100644 --- a/bin/isolated-functions.sh +++ b/bin/isolated-functions.sh @@ -17,7 +17,7 @@ assert() { done } -__assert_sigpipe_ok() { +__helpers_assert_sigpipe_ok() { # When extracting a tar file like this: # # bzip2 -dc foo.tar.bz2 | tar xof - @@ -32,15 +32,19 @@ __assert_sigpipe_ok() { # which says, "When a command terminates on a fatal # signal whose number is N, Bash uses the value 128+N # as the exit status." - local x pipestatus=${PIPESTATUS[*]} - for x in $pipestatus ; do + for x in ${pipestatus} ; do # Allow SIGPIPE through (128 + 13) - [[ $x -ne 0 && $x -ne ${PORTAGE_SIGPIPE_STATUS:-141} ]] && die "$@" + if [[ ${x} -ne 0 && ${x} -ne ${PORTAGE_SIGPIPE_STATUS:-141} ]] ; then + __helpers_die "${FUNCNAME}: error ${x}: $*" + return ${x} + fi done # Require normal success for the last process (tar). - [[ $x -eq 0 ]] || die "$@" + [[ ${x} -eq 0 ]] || __helpers_die "${FUNCNAME}: error ${x}: $*" || return ${x} + + return 0 } shopt -s extdebug @@ -92,26 +96,75 @@ nonfatal() { die "$FUNCNAME() not supported in this EAPI" fi if [[ $# -lt 1 ]]; then - die "$FUNCNAME(): Missing argument" + # Well they said nonfatal and forgot to finish their sentence. + # Something ain't right, to be sure, but die()ing here seems a bit + # pendantic and mean-spirited :) + local phase_str= + [[ -n ${EBUILD_PHASE} ]] && phase_str=" (${EBUILD_PHASE} phase)" + eerror "ERROR: ${CATEGORY}/${PF}${phase_str}:" + eerror " ${FUNCNAME}: Missing argument" + eerror + return 1 fi PORTAGE_NONFATAL=1 "$@" } +# Memorizing the return-code at the beginning of __helpers_die() +# and returning it unchanged (unless it's 0) is intended as a +# courtesy to invokers who may use a bash sequence like: +# +# failure_prone_action || \ +# __helpers_die "${FUNCNAME}: descriptive error message" || \ +# return $? +# +# as syntactic sugar for the following equivalent code: +# +# failure_prone_action +# ret=$? +# if [[ ${ret} -ne 0 ]] ; then +# __helpers_die "${FUNCNAME}: descriptive error message" +# return ${ret} +# fi +# +# However, beware: this trick will not work for positive +# conditionals because __helpers_die always returns a nonzero +# failure code (if it returns at all). Neither +# +# action_intended_to_fail && __helpers_die msg && return $? +# +# nor +# +# action_intended_to_fail && __helpers_die msg || return $? +# +# will work as intended. The following would be a clean, correct +# way to trigger __helpers_die() from a positive conditional: +# +# action_intended_to_fail && { +# __helpers_die "${FUNCNAME}: descriptive error message" +# return 1 +# } +# # NB: if this were the last command in your helper function, you +# # should "return 0" here, as $? is always nonzero at this point. +# __helpers_die() { - if ___eapi_helpers_can_die; then + local retcode=$? + (( retcode == 0 )) && retcode=1 + + if ___eapi_helpers_can_die && [[ "${PORTAGE_NONFATAL:-0}" != 1 ]]; then die "$@" else - echo -e "$@" >&2 + local phase_str= + [[ -n ${EBUILD_PHASE} ]] && phase_str=" (${EBUILD_PHASE} phase)" + ewarn "WARNING: ${CATEGORY}/${PF}${phase_str}:" + ewarn " ${FUNCNAME}: ${*:-(no error message)}" + ewarn fi + + return "${retcode}" } die() { - if [[ $PORTAGE_NONFATAL -eq 1 ]]; then - echo -e " $WARN*$NORMAL ${FUNCNAME[1]}: WARNING: $@" >&2 - return 1 - fi - set +e if [ -n "${QA_INTERCEPTORS}" ] ; then # die was called from inside inherit. We need to clean up diff --git a/bin/phase-helpers.sh b/bin/phase-helpers.sh index 5055060..85fa011 100644 --- a/bin/phase-helpers.sh +++ b/bin/phase-helpers.sh @@ -278,7 +278,7 @@ unpack() { local y local myfail local eapi=${EAPI:-0} - [ -z "$*" ] && die "Nothing passed to the 'unpack' command" + [ -z "$*" ] && { __helpers_die "Nothing passed to the 'unpack' command" ; return 1 ; } for x in "$@"; do __vecho ">>> Unpacking ${x} to ${PWD}" @@ -288,65 +288,73 @@ unpack() { if [[ ${x} == "./"* ]] ; then srcdir="" elif [[ ${x} == ${DISTDIR%/}/* ]] ; then - die "Arguments to unpack() cannot begin with \${DISTDIR}." + __helpers_die "Arguments to unpack() cannot begin with \${DISTDIR}." + return 1 elif [[ ${x} == "/"* ]] ; then - die "Arguments to unpack() cannot be absolute" + __helpers_die "Arguments to unpack() cannot be absolute" + return 1 else srcdir="${DISTDIR}/" fi - [[ ! -s ${srcdir}${x} ]] && die "${x} does not exist" + [[ -s ${srcdir}${x} ]] || __helpers_die "${x} does not exist" || return 1 __unpack_tar() { if [ "${y}" == "tar" ]; then $1 -c -- "$srcdir$x" | tar xof - - __assert_sigpipe_ok "$myfail" + __helpers_assert_sigpipe_ok "$myfail" || return $? else local cwd_dest=${x##*/} cwd_dest=${cwd_dest%.*} - $1 -c -- "${srcdir}${x}" > "${cwd_dest}" || die "$myfail" + $1 -c -- "${srcdir}${x}" > "${cwd_dest}" || \ + __helpers_die "$myfail" || return $? fi } myfail="failure unpacking ${x}" case "${x##*.}" in tar) - tar xof "$srcdir$x" || die "$myfail" + tar xof "$srcdir$x" || __helpers_die "$myfail" || return $? ;; tgz) - tar xozf "$srcdir$x" || die "$myfail" + tar xozf "$srcdir$x" || __helpers_die "$myfail" || return $? ;; tbz|tbz2) ${PORTAGE_BUNZIP2_COMMAND:-${PORTAGE_BZIP2_COMMAND} -d} -c -- "$srcdir$x" | tar xof - - __assert_sigpipe_ok "$myfail" + __helpers_assert_sigpipe_ok "$myfail" || return $? ;; ZIP|zip|jar) # unzip will interactively prompt under some error conditions, # as reported in bug #336285 ( set +x ; while true ; do echo n || break ; done ) | \ - unzip -qo "${srcdir}${x}" || die "$myfail" + unzip -qo "${srcdir}${x}" + __helpers_assert_sigpipe_ok "$myfail" || return $? ;; gz|Z|z) - __unpack_tar "gzip -d" + __unpack_tar "gzip -d" || return $? ;; bz2|bz) - __unpack_tar "${PORTAGE_BUNZIP2_COMMAND:-${PORTAGE_BZIP2_COMMAND} -d}" + __unpack_tar "${PORTAGE_BUNZIP2_COMMAND:-${PORTAGE_BZIP2_COMMAND} -d}" || \ + return $? ;; 7Z|7z) local my_output my_output="$(7z x -y "${srcdir}${x}")" if [ $? -ne 0 ]; then echo "${my_output}" >&2 - die "$myfail" + __helpers_die "${myfail}" || return $? fi ;; RAR|rar) - unrar x -idq -o+ "${srcdir}${x}" || die "$myfail" + unrar x -idq -o+ "${srcdir}${x}" || \ + __helpers_die "${myfail}" || return $? ;; LHa|LHA|lha|lzh) - lha xfq "${srcdir}${x}" || die "$myfail" + lha xfq "${srcdir}${x}" || \ + __helpers_die "${myfail}" || return $? ;; a) - ar x "${srcdir}${x}" || die "$myfail" + ar x "${srcdir}${x}" || \ + __helpers_die "${myfail}" || return $? ;; deb) # Unpacking .deb archives can not always be done with @@ -356,31 +364,35 @@ unpack() { # installed. if type -P deb2targz > /dev/null; then y=${x##*/} - local created_symlink=0 - if [ ! "$srcdir$x" -ef "$y" ] ; then + if [ ! "${srcdir}${x}" -ef "${y}" ] ; then # deb2targz always extracts into the same directory as # the source file, so create a symlink in the current # working directory if necessary. - ln -sf "$srcdir$x" "$y" || die "$myfail" - created_symlink=1 - fi - deb2targz "$y" || die "$myfail" - if [ $created_symlink = 1 ] ; then + ln -sf "${srcdir}${x}" "${y}" || \ + __helpers_die "${myfail}" || return $? + deb2targz "${y}" || \ + __helpers_die "${myfail}" || return $? # Clean up the symlink so the ebuild # doesn't inadvertently install it. - rm -f "$y" + rm -f "${y}" || \ + __helpers_die "${myfail}" || return $? + else + deb2targz "${y}" || \ + __helpers_die "${myfail}" || return $? fi - mv -f "${y%.deb}".tar.gz data.tar.gz || die "$myfail" + mv -f "${y%.deb}".tar.gz data.tar.gz || \ + __helpers_die "${myfail}" || return $? else - ar x "$srcdir$x" || die "$myfail" + ar x "${srcdir}${x}" \ + || __helpers_die "${myfail}" || return $? fi ;; lzma) - __unpack_tar "lzma -d" + __unpack_tar "lzma -d" || return $? ;; xz) if ___eapi_unpack_supports_xz; then - __unpack_tar "xz -d" + __unpack_tar "xz -d" || return $? else __vecho "unpack ${x}: file format not recognized. Ignoring." fi @@ -392,8 +404,8 @@ unpack() { done # Do not chmod '.' since it's probably ${WORKDIR} and PORTAGE_WORKDIR_MODE # should be preserved. - find . -mindepth 1 -maxdepth 1 ! -type l -print0 | \ - ${XARGS} -0 chmod -fR a+rX,u+w,g-w,o-w + find . -mindepth 1 -maxdepth 1 ! -type l -execdir chmod -fR a+rX,u+w,g-w,o-w '{}' + || \ + __helpers_die "${myfail}" || return $? } econf() { @@ -404,7 +416,7 @@ econf() { fi __hasg() { - local x s=$1 + local x s="$1" shift for x ; do [[ ${x} == ${s} ]] && echo "${x}" && return 0 ; done return 1 @@ -430,7 +442,8 @@ econf() { if [[ -n $CONFIG_SHELL && \ "$(head -n1 "$ECONF_SOURCE/configure")" =~ ^'#!'[[:space:]]*/bin/sh([[:space:]]|$) ]] ; then sed -e "1s:^#![[:space:]]*/bin/sh:#!$CONFIG_SHELL:" -i "$ECONF_SOURCE/configure" || \ - die "Substition of shebang in '$ECONF_SOURCE/configure' failed" + __helpers_die "Substition of shebang in '$ECONF_SOURCE/configure' failed" || \ + return $? fi if [ -e "${EPREFIX}"/usr/share/gnuconfig/ ]; then find "${WORKDIR}" -type f '(' \ diff --git a/bin/save-ebuild-env.sh b/bin/save-ebuild-env.sh index f6dc2c5..e19f10a 100644 --- a/bin/save-ebuild-env.sh +++ b/bin/save-ebuild-env.sh @@ -46,7 +46,7 @@ __save_ebuild_env() { done unset x - unset -f assert __assert_sigpipe_ok \ + unset -f assert __helpers_assert_sigpipe_ok \ __dump_trace die \ __quiet_mode __vecho __elog_base eqawarn elog \ einfo einfon ewarn eerror ebegin __eend eend KV_major \ diff --git a/doc/package/ebuild/eapi/4.docbook b/doc/package/ebuild/eapi/4.docbook index 8dd0f14..0b49635 100644 --- a/doc/package/ebuild/eapi/4.docbook +++ b/doc/package/ebuild/eapi/4.docbook @@ -11,9 +11,14 @@ The dohard and dosed helpers from previous EAPIs are no longer available.
All helpers die on failure -All helpers now die automatically whenever some sort of error occurs. -Helper calls may be prefixed with the 'nonfatal' helper in order -to prevent errors from being fatal. +All helpers now die automatically whenever any meaningful error occurs. +The 'nonfatal' helper may be used to effect the error-handling behavior +of EAPIs 0-3, in which recoverable errors emit a warning and return +nonzero exit-codes to invokees. It is used by prepending 'nonfatal' to any helper +invocation; for example, by replacing 'dodoc /foo.txt' with 'nonfatal dodoc /foo.txt'. +The 'nonfatal' helper should not be applied to +statements other than helper invocations, and has no effect on the 'die' +and 'assert' helpers.