Re: [PATCH] modernize AS_UNSET a bit (take 2)

[Eric Blake]

Paolo Bonzini <bonzini <at> gnu.org> writes:

> The last one for today, and hopefully the last before shell functions
> are actually used.  This patch takes into account Ralf's worries about
> unset portability.

Indeed, if we are requiring shell functions, then
 http://www.in-ulm.de/~mascheck/bourne/
confirms that unset is available, and we only have to avoid its bugs.  We need 
to update the manual accordingly (ie. where we currently mention System7 as the 
portable baseline, we should add a link to the above site, and add mention of 
the fact that in practice, m4sh guarantees SVR2 functionality rather than 
System7 as the baseline).

> 2008-10-14  Paolo Bonzini  <bonzini <at> gnu.org>
> 
>       * lib/m4sugar/m4sh.m4 (_AS_UNSET_PREPARE): Provide $as_unset as an
>       alias for AS_UNSET, for backwards compatibility.

We need one more iteration before this can be applied.

> +++ b/lib/autoconf/programs.m4
> @@ -894,7 +894,7 @@ AC_DEFUN([AC_PROG_SED],
>         ac_script="$ac_script$as_nl$ac_script"
>       done
>       echo "$ac_script" 2>/dev/null | sed 99q >conftest.sed
> -     $as_unset ac_script || ac_script=
> +     ac_script=; AS_UNSET([ac_script])

Why the extra assignment to the empty string prior to the unset?  This should 
probably be just:

AS_UNSET([ac_script])

>  m4_defun([_AS_UNSET_PREPARE],
> -[# Support unset when possible.
> -if ( (MAIL=60; unset MAIL) || exit) >/dev/null 2>&1; then
> -  as_unset=unset
> -else
> -  as_unset=false
> -fi
> +[AS_REQUIRE_SHELL_FN([as_func_unset], [AS_UNSET([$1])])dnl

This should be AS_UNSET([$[1]]), as you want a literal $1 in the body of the 
shell function, rather than an empty string when _AS_UNSET_PREPARE is called 
without arguments.

>  m4_defun([AS_UNSET],
> -[AS_REQUIRE([_AS_UNSET_PREPARE])dnl
> -$as_unset $1 || test "${$1+set}" != set || { $1=$2; export $1; }])
> +[{ unset $1 >/dev/null 2>&1 || :;}])

This does not squelch error messages about attempts to unset a read-only 
variable, at least on pdksh.  But since that shell also aborts the script at 
that point, I guess I'm okay with the failure; in general, marking critical 
variables read-only is the user's problem (we can't neutralize them, at any 
rate).  On the other hand, Solaris /bin/sh does squelch the error, and yet 
still aborts the script.  So do we really want to be silencing stderr here, 
since it is a bad idea for a script to fail with non-zero status without 
explaining why?

$ pdksh -c 'foo=a;readonly foo;unset foo 2>/dev/null; echo $foo'; echo $?
ksh: foo: is read only
1
$ ash -c 'foo=a;readonly foo;unset foo 2>/dev/null; echo $foo'; echo $?
a
0
$ /bin/sh -c 'foo=a;readonly foo;unset foo 2>/dev/null; echo $foo'; echo $?
1
$

> 
> +# For backwards compatibility.
> +AS_REQUIRE([_AS_UNSET_PREPARE])
> +

Is this strictly necessary, considering $as_unset was undocumented?  Any uses 
in configure.ac are okay (because configure already uses AS_PREPARE), so we 
only have to worry about independent m4sh scripts.  Hmmm, looking further at 
libtool's usage patterns - at least libtoolize.m4sh calls

AS_SHELL_SANITIZE
$as_unset CDPATH

So, even though CDPATH is already unset (by virtue of AS_SHELL_SANITIZE), the 
presence of this line would cause libtoolize to complain about "CDPATH: command 
not found" if we don't provide the backwards compatibility.  Oh well.

-- 
Eric Blake