[patch] Fix nonfatal, die, and helper code to comport with PMS

portage_fix_nonfatal_die_spin2.patch (text/plain), 14.73 KB, created by Greg Turner on 2012-11-12 12:06:29 UTC

commit 1888bd198b938a8d2cbb109ba9cf1bb6c09b6e6f
Author: Gregory M. Turner <[e-mail]>
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 <[e-mail]>

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.
 <section id='package-ebuild-eapi-4-helpers-die-nonfatal'>
 <title>All helpers die on failure</title>
 <para>
-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.
 </para>
 </section>
 <section id='package-ebuild-eapi-4-helpers-docompress'>