Re: [PATCH 2/5] configure: search a sturdy POSIX shell to be used in the testsuite

[Stefano Lattarini]

On 05/07/2012 05:37 PM, Eric Blake wrote:
> On 05/01/2012 10:04 AM, Stefano Lattarini wrote:
>> * configure.ac: Add code (partially inspired to checks in gnulib's
>> 'tests/init.sh') to search for a good-enough, not-buggy POSIX/XSI
>> shell to be used in our testsuite.  Accordingly AC_SUBSTitute the
>> variable 'AM_TEST_RUNNER_SHELL'.
>> * NEWS: Update.
>>
>> Signed-off-by: Stefano Lattarini <address@hidden>
>> ---
>>  NEWS         |   11 ++++
>>  configure.ac |  190 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 184 insertions(+), 17 deletions(-)
>>
> 
>>  
>> -# Shell used to run our test scripts.  The same as $SHELL/$CONFIG_SHELL
>> -# for the moment.
>> -AC_SUBST([AM_TEST_RUNNER_SHELL], [$SHELL])
>> +dnl FIXME: could we extract this in a simpler way through autoconf
>> +dnl FIXME: idioms or internals?
>> +AC_DEFUN(
>> +  [_AM_INIT_BOURNE_COMPATIBLE_VAR],
>> +  [am_bourne_compatible="AS_ESCAPE(_m4_expand([AS_BOURNE_COMPATIBLE]))"])
> 
> As is, _m4_expand is already an autoconf internal.
>
Yes, but the way we are using it is not simple enough IMHO.  When we abuse
Autoconf's internal (like here), we should ideally have a bigger gain ;-)

> Thankfully, since you recently moved to requiring 2.65,
>
Oh, but the above is only meant to be used by the Automake's own build system
(not by any Automake-provided public m4 macros), so that we can safely assume
the presence of Autoconf 2.69.

> it looks like this macro is safe enough for you to use.
>
And even if it's not, this is a problem that would bite only the Automake
developers (which regenerate configure), not the Automake users.

>> +
>> +dnl
>> +dnl Arguments to this macro:
>> +dnl
>> +dnl   $1 - shell to test
>> +dnl   $2 - description of the tested feature
>> +dnl   $3 - shell code used to check the feature; to indicate success,
>> +dnl        it can either exit with status 77, or have the last command
>> +dnl        returning with exit status of zero
>> +dnl   $4 - shell code to execute if the check on the shell is successful
>> +dnl        (defaults to nothing)
>> +dnl   $5 - shell code to execute if the check on the shell is not
>> +dnl        successful (defaults to nothing)
>> +dnl
>> +AC_DEFUN([_AM_CHECK_SHELL_FEATURE],
>> +  [AC_REQUIRE([_AM_INIT_BOURNE_COMPATIBLE_VAR])
>> +  AC_MSG_CHECKING([whether $1 $2])
>> +  if { $1 -c "$am_bourne_compatible
>> +AS_ESCAPE([$3])
>> +test \$? -eq 0 || exit 1
>> +# Use 77 to indicate success (rather than 0), in case some shell
>> +# acts like Solaris 10's /bin/sh, exiting successfully on some
>> +# syntax errors.
>> +exit 77" >&AS_MESSAGE_LOG_FD 2>&1; test $? -eq 77; }
>> +  then
>> +    AC_MSG_RESULT([yes])
>> +    $4
>> +  else
>> +    AC_MSG_RESULT([no])
>> +    $5
>> +  fi])
> 
> Looks like as good a probe as we can do without something publicly
> documented in autoconf for the same.
>
Well, I won't make mistery of the fact that I hope the probes I implemented
in this series will eventually find their way into Autoconf proper, freeing
the Automake developers from the need to maintain and test them ...

> 
>> +#
>> +# Finally, we look for weird bugs and portability problems mentioned in
>> +# the Autoconf manual, and reject shells that suffers from them. (TODO)
> 
> Any reason to leave the TODO comment here?
>
The fact that we haven't yet written any actual code to reject those shells? ;-)

>> +    _AM_CHECK_SHELL_FEATURE([$1],
>> +      [supports address@hidden:@glob}],
>> +      [v=a/b/c; test address@hidden:@*/} = b/c],
>> +      [], [am_score=1; break])
>> +
>> +    _AM_CHECK_SHELL_FEATURE([$1],
>> +      [supports address@hidden:@@%:@glob}],
>> +      [v=a/b/c; test address@hidden:@@%:@*/} = c],
>> +      [], [am_score=1; break])
>> +
>> +    _AM_CHECK_SHELL_FEATURE([$1],
>> +      [supports \${var%glob}],
>> +      [v=a.b.c; test ${v%.*} = a.b],
>> +      [], [am_score=1; break])
>> +
>> +    _AM_CHECK_SHELL_FEATURE([$1],
>> +      [supports \${var%%glob}],
>> +      [v=a.b.c; test ${v%%.*} = a],
>> +      [], [am_score=1; break])
> 
> Probably overkill to test all four; I don't know of any shell that
> supports one of the XSI expansions, but not all four.
>
You are very likely correct, but I'd rather err on the side of safety for
now.

> 
>> +AC_CACHE_VAL(
>> +  [ac_cv_AM_TEST_RUNNER_SHELL],
>> +  [if test "$AM_TEST_RUNNER_SHELL"; then
>> +    # Let the user override it.
>> +    ac_cv_AM_TEST_RUNNER_SHELL=$AM_TEST_RUNNER_SHELL
>> +  else
>> +    ac_cv_AM_TEST_RUNNER_SHELL=no
>> +    am_candidate_shells=${CONFIG_SHELL-}
>> +    # For the benefit of Solaris.
>> +    am_PATH=$PATH$PATH_SEPARATOR/usr/xpg4/bin$PATH_SEPARATOR/usr/xpg6/bin
> 
> This favors the user's PATH first, which seems reasonable (first
> candidate on the user's path that does what we want).  But in the
> fallback mode, wouldn't it be better to list xpg6 before xpg4, for the
> additional fixes in xpg6?
>
Good point, consider this fixed (although this is just a theoretical issue,
since I don't know of any system that actually has a shell in /usr/xpg6/bin).

> Overall, looks reasonable for finding a decent shell.
> 
Thanks.  I've pushed the patch now.

Regards,
  Stefano