[gentoo-dev] Thoughts on try()

[Aron Griffis]

Hello,

This email follows a discussion on #gentoo yesterday.  I'll start with
an executive summary of what follows since this may become long-winded.
Feedback and comments are welcome.

Summary
-------
  try() (in ebuild.sh) attempts to provide ebuilds with an easy
  mechanism for aborting the build in case a single step fails.  Its
  arguments need to be interpreted as a bash command, including
  overriding environment variables, bash builtins, and external
  programs.  Quoting needs to be preserved.

  To accomplish this, it needs to use either bash "eval" or the external
  program "env".  However each of these methods has shortcomings, and
  neither handles pipelines or redirection well.  As a result, I propose
  an alternate method that is more suited to bash syntax: die() and
  assert_true_or_die().

Defining the Problem
--------------------
  Ebuild authors need an easy method for "throwing an exception" in an
  ebuild so that ebuilds can be robust.  Non-zero exit status from
  a command needs to detected, a standard error message given to the
  user, and the build aborted.

  Constraints:

    (1) The "command" should be written as a normal bash command, with
	normal quoting, etc.  In other words, it should be possible to
	tack the method onto a command with minimal effort.

    (2) The "command" can be a bash builtin or external program (or
	function for that matter), and should be able to contain
	overridden environment variables as bash normally allows.

Initial solution: try()
-----------------------
  try() was written to follow the pattern of languages with exception
  handling.  It's mission seems simple enough: Interpret the arguments
  as a bash command, execute the command, abort with an error message if
  the command fails.

  The problem with try() is the incompatibility of the two constraints
  in bash.  Consider the following list of problems with some
  skeleton implementations of try():

    * If try() { eval "$@"; } is used, then the arguments to try()
      must be escaped so that the quoting is preserved.

    * If try() { env "$@"; } is used, then the command can't be a bash
      builtin, because env doesn't understand bash builtins.

    * If try() { bash -c "$@"; } is used, then the effects of the
      builtin won't affect the parent shell, e.g. cd and setting of
      variables.

    * If try() { "$@"; } is used, then bash doesn't evaluate the line
      for prefixed setting of environment variables.

  However this is only the tip of the iceberg!  Here are some more
  problems that use the following try(), functionally similar to the
  one in ebuild.sh:

    try() { env "$@"; if [ $? != 0 ]; then echo "ERROR"; exit 1; fi; }
  
    * Since redirections occur *outside* of the try() function call
      (programs executed inside of the function inherit the file
      descriptors), they won't cause try() to fail.  For example:

	$ try cat nonexistent_file
	ERROR (then shell exits)

	$ try cat < nonexistent_file
	bash: nonexistent_file: No such file or directory

    * Continuing on the redirections thread, consider what happens when
      you redirect the stderr of the command...  The ebuild will bomb
      out, but the error message will be lost because try() no longer
      has access to the output terminal.  (Yes, it could use /dev/tty,
      but shouldn't because that would pose problems for automated
      builds.)

	$ try grep patt * >/dev/null 2>&1
	[no error message from try]

    * try() only applies to the first element of a pipeline.  As
      a result, the following would not be caught by try().

	$ try cat bogus_but_existing_patch | patch -p1
	patch: **** Only garbage was found in the patch input.

  I think try() is a good idea, but the limitations of bash syntax make
  it difficult to implement well.  Therefore I propose a different
  solution...

Proposed replacement: die() and assert_true_or_die()
----------------------------------------------------
  diefunc() {
    local funcname="$1" lineno="$2" exitcode="$3"
    shift 3
    echo &>2
    echo "!!! ERROR: The ebuild did not complete successfully." &>2
    echo "!!! Function $funcname, Line $lineno, Exitcode $exitcode" &>2
    echo "!!! ${*:-(no error message)}" &>2
    echo &>2
    exit 1
  }
  alias die='diefunc "$FUNCNAME" "$LINENO" "$?"'
  alias assert_true_or_die='_retval=$?; [ $_retval = 0 ] || \
                            diefunc "$FUNCNAME" "$LINENO" "$_retval"'

  The aliases are necessary to get FUNCNAME and LINENO from the
  caller rather than the callee (diefunc).

  Note one thing (other than usage) that changes between try() and
  die().  The error output of try() shows the actual command that was
  attempted.  That isn't possible with die() because it isn't being
  passed the command.  However it attempts to make up the shortcoming by
  including the function name, the line number, the exitcode, and
  (bonus) a custom message that can be passed by the caller.

  To convert try-lines to die-lines would be as follows (each line
  followed by its conversion).  These examples are all from the Linux
  kernel ebuild.

    try mv linux linux-${KV}

    mv linux linux-${KV} || die

    ----------------------------------------

    try bzip2 -dc ${DISTDIR}/patch-${KV}.bz2 | patch -p1

    bzip2 -dc ${DISTDIR}/patch-${KV}.bz2 | patch -p1 || die

    # or for checking the first half of the pipeline as well as the
    # second half, since bash only reports exit status from the last
    # element on a pipeline...
    (bzip2 -dc ${DISTDIR}/patch-${KV}.bz2 || die) | patch -p1 || die

    ----------------------------------------

    try cd LVM/${LVMV}

    # include a custom message
    cd LVM/${LVMV} || die "Looks like LVM didn't unpack correctly."

    ----------------------------------------

    try CFLAGS="${CFLAGS} -I${S}/include" ./configure --prefix=/ \
	--mandir=/usr/share/man --with-kernel_dir="${S}"

    # use the assert_true_or_die() version for clarity
    CFLAGS="${CFLAGS} -I${S}/include" ./configure --prefix=/ \
	--mandir=/usr/share/man --with-kernel_dir="${S}" 
    assert_true_or_die "Kernel configuration failed"

    ----------------------------------------

    try make KERNEL_VERSION=${KV} KERNEL_DIR=${S}

    make KERNEL_VERSION=${KV} KERNEL_DIR=${S} || die

Conclusion
----------
  I believe that die() and assert_true_or_die() fulfill the constraints,
  provide a workable replacement for try(), and are more bashish all
  around.  Note there is no need to immediately convert all existing
  ebuilds to die() and assert_true_or_die().  If this proposal is
  accepted (after discussion and potential modifications), then the new
  functions should be used in new ebuilds, and old ebuilds can be
  gradually changed to the new scheme.

  Feedback/questions/comments are welcome!  Thanks for reading.

Aron