Re: [PATCH] Generic mechanism to detect shell features

[Eric Sunshine]

Hello,

On Mon, 12 Jan 2004 14:21:37 +0100, bonzini wrote:
> This patch adds to m4sh a generic mechanism to detect required or
> desirable shell features, and when looping over the shells, avoid shells
> that do not require the former and prefer shells that have the latter.
> The idea is to pass tests to AS_DETECT_REQUIRED and AS_DETECT_SUGGESTED:
> for now both $LINENO tests and shell functions tests go into
> AS_DETECT_SUGGESTED, but as soon as they will be used functions will
> automatically be required.

The notion of a generic detection mechanism is a nice one.  Unfortunately, I  
see a number of problems with the patch, some of which are fatal.

> +m4_define([_AS_DETECT_REQUIRED_BODY], [])
> +m4_define([_AS_DETECT_REQUIRED_FEATURES],
> +[{ _AS_QUOTE([$1]) <<\_ASEOF
> +_AS_BOURNE_COMPATIBLE
> +_AS_DETECT_REQUIRED_BODY
> +_ASEOF
> +} >/dev/null 2>&1])

Problem 1: When there are no tests in _AS_DETECT_REQUIRED_BODY (which is the  
case presently), then this returns the result of whatever the last command  
in _AS_BOURNE_COMPATIBLE returns.  This is highly dangerous since it creates  
a very tight coupling between _AS_BOURNE_COMPATIBLE and  
_AS_DETECT_REQUIRED_FEATURES.  If someone makes a change to  
_AS_BOURNE_COMPATIBLE which causes 'false' to be returned from its last  
command, then _AS_DETECT_REQUIRED_FEATURES will start failing mysteriously.   
To fix, you should ensure that _AS_DETECT_REQUIRED_FEATURES results in 'true'  
when _AS_DETECT_REQUIRED_BODY contains no tests.

> +# _AS_DETECT_BETTER_SHELL
> + AS_IF([_AS_DETECT_REQUIRED_FEATURES([$SHELL])], [as_have_required=yes],

Problem 2: This code is still making the fundamentally flawed assumption  
that SHELL is the shell running the script.  This very frequently is not the  
case.  Worse, it is incorrectly applying the results of the SHELL tests to  
the shell running the script.

Keep in mind that SHELL is imported from the environment, it is not  
typically set automatically to the shell which is running the script.  I gave  
a couple examples in previous emails illustrating why you can not rely upon  
SHELL being the shell running the script.  For instance, in my environment,  
SHELL is set to tcsh, yet when I run the configure script it runs via /bin/sh  
on account of the #!/bin/sh.  Another example is that someone might have  
SHELL set to /usr/local/bin/bash, yet the shell running the script will still  
(by default) be /bin/sh (because of #!/bin/sh).  (Paul Jarc was nice enough  
to do some testing, and he discovered that only bash and Solaris ksh will set  
SHELL automatically to the running shell, but _only_ if SHELL is not already  
present in the environment.)

The upshot is that you simply can not rely upon SHELL being the shell which  
is presently running the script.  Consequently, the feature detection code  
needs to be reorganized (and possibly refactored) so that it first tests the  
currently running shell, then SHELL, and finally the shells discovered by the  
more exhaustive search.  (This is the approach I took with the patch I  
submitted.)

> AS_IF([test $as_have_required = yes &&
> _AS_DETECT_SUGGESTED_FEATURES([$SHELL])],
> +    [CONFIG_SHELL="$SHELL"],
> +    [_AS_PATH_WALK([...],
> +        for as_base in sh sh5 ksh zsh bash; do
> +          if test -f $as_dir/$as_base; then
> +            AS_IF([_AS_DETECT_REQUIRED_FEATURES([$as_dir/$as_base])],
> +                  [CONFIG_SHELL=$as_dir/$as_base

Problem 3: Inconsistency.  The code sets CONFIG_SHELL to SHELL if SHELL  
passes _both_ required and suggested feature tests; yet it sets CONFIG_SHELL  
to one of the other shells (sh, sh5, ksh, zsh, bash) if it passes only the  
required feature test.  Why the inconsistency?  Why are the requirements  
placed upon SHELL more strict than the requirements for the other shells?

Aside: I see that you changed the order of the shell tests to "sh sh5 ksh  
zsh bash".  Historically, it has been "sh bash ksh sh5".  Is this  
intentional? My own experience is that the historical search order gives  
better results.

> + as_have_required_yes

Problem 4: Syntax error.  I assume that you meant "as_have_required=yes".

> +    if test $as_have_required = no; then
> +      echo This script requires a shell more modern than all the
> +      echo shells that I found on your system.  Please install a
> +      echo modern shell, or manually run the script under such a
> +      echo shell if you do have one.
> +      AS_EXIT(1)
> +    fi

Problem 5: Because of the way this is nested inside the 'if' block, this  
error message will be emitted only if none of the externally discoverd shells  
(sh bash ksh sh5) support the required features.  It will _fail_ to emit  
this message for SHELL if SHELL does not implement the required features,  
since the results of the SHELL test are incorrectly applied to the running  
shell, as discussed for problem #2.

> +    if test "$SHELL" != "$CONFIG_SHELL"; then
> +      AS_UNSET([ENV])
> +      AS_UNSET([BASH_ENV])
> +      export CONFIG_SHELL
> +      exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
> +    fi])

Problem 6: This is invalid logic, as discussed for problem #2.  SHELL is not  
guaranteed to be the running shell, thus this test is incorrect.  Worse, if  
SHELL does support required and suggested features, but the running shell  
does not, then the script will not re-exec with SHELL, but will instead  
continue running under the crippled shell.

>  # _AS_LINENO_PREPARE
> m4_define([_AS_LINENO_PREPARE],
> [AS_REQUIRE([_AS_CR_PREPARE])dnl
> +AS_DETECT_SUGGESTED([_AS_LINENO_WORKS])

Problem 7: I have not yet tracked down the exact cause of this problem, but  
in the generated configure script, wherever _AS_DETECT_SUGGESTED_FEATURES()  
has been invoked, the _AS_LINENO_WORKS test is emitted twice.  For instance,  
in the generated configure script, I see this:

  { {
    as_lineno_1=$LINENO
    ...
  } } || { { (exit 1); exit 1; } }
  { {
    as_lineno_1=$LINENO
    ...
  } } || { { (exit 1); exit 1; } }
  _ASEOF

The _AS_SHELL_FN_WORK test, on the other hand, is only emitted once per  
invocation of _AS_DETECT_SUGGESTED_FEATURES(), which is correct.

> +# AS_SHELL_FN_SPY
> +m4_define([AS_SHELL_FN_SPY],
> +{ $SHELL 2> /dev/null <<\_ASEOF
> +_AS_SHELL_FN_WORK
> +_ASEOF
> +} || {
> +  echo No shell found that supports shell functions.
> +}])])

Problem 8: As with problem #2, this is making the invalid assumptiong that  
SHELL is the running shell.  This "spy" is supposed to be reporting that the  
running shell does not support functions, but instead it is reporting that  
SHELL does not support functions.

>  # AS_INIT(WHAT-IF-SHELL-FUNCTIONS-DO-NOT-WORK)
>  # --------------------------------------------
>  # The parameter is temporary; it will go away and you

You forgot to update the prototype and comment after eliminating the  
argument from AS_INIT.

-- ES