Re: [PATCH] Shell function reporting

[Eric Sunshine]

On 07 Jan 2004 10:15:02 -0800, Paul Eggert wrote:
> Eric Sunshine <address@hidden> writes:
> > CONFIG_SHELL=try_to_find_suitable_shell()
> > if CONFIG_SHELL is set
> At this point, CONFIG_SHELL must support functions, right?

If CONFIG_FILE is set, then yes it will reference a shell which supports 
functions.

> > Assuming the $SHELL test fails, the first time through CONFIG_SHELL is
> > not set, so it tries to find a suitable shell. If it finds one, then it
> > exports CONFIG_SHELL re-invokes the script. Now, the second time through,
> > CONFIG_SHELL is set, so it falls down to the '*' case
> I don't see how.  The 2nd time through, $CONFIG_SHELL must support
> functions so the code must execute the "Yay!" comment only.

No, the SHELL variable remains unaltered.  The only variable which the code  
is changing is CONFIG_SHELL.  Therefore, the "Yay!" is not printed, and it  
falls into the else caluse, and finally into the "*)" case which emits the  
warning about not finding a function-supporting shell.

> Unless CONFIG_SHELL differs from SHELL the 2nd time through, i.e.
> they identify different shells the 2nd time through?  Then that would
> explain your problem; but in that case it seems to me that we might
> have more serious problems since SHELL is incorrect, and we should fix
> SHELL somehow.

Indeed, that generally is a problem, but I did not want to mess with that  
since I am not familiar with the original reasoning behind introduction of  
CONFIG_SHELL.  It seems more correct to me to set and use SHELL throughout,  
rather than introducing the CONFIG_SHELL convolution.

> (There is another problem here: the current code is testing SHELL
> rather than CONFIG_SHELL.

My original submission fixed this problem.  In that patch, I explicitly  
ensured that CONFIG_SHELL was tested for function support, however that patch  
was rejected.

> It is appropriate to test SHELL since that
> (presumably) is the shell that is running this script.

Actually, it's not, and this represents yet another bug in the existing  
code.  There is no guarantee whatsoever that SHELL is the shell which is  
running the script.  SHELL is imported from the environment, and it may point  
to something quite different from the shell invoking the script.  For  
instance, in my environment, SHELL is set to tcsh, yet when I invoke  
configure, it is run by /bin/sh (because of #! /bin/sh at the top of the  
script).

> But if SHELL
> and CONFIG_SHELL differ, the code won't be testing CONFIG_SHELL, even
> though perhaps it should be (if only to warn the user that the
> CONFIG_SHELL setting may have problems).  But this is a different
> matter than the one you're trying to to address.)

I did address it originally, but after further consideration it seemed like  
a better idea to not do so; to allow the user to shoot himself in the foot if  
he so chose.  In other words, if a user explicitly sets CONFIG_SHELL, then  
we should probably assume that he knows what he's doing and not try to second  
guess him.  I see CONFIG_SHELL as a deeply private implementation detail of  
the configure script, so if a user is mucking around with it, then on his own  
head be it.

> > One way to correct the logic is as follows:
> > case $CONFIG_SHELL in
> > CONFIG_SHELL=try_to_find_suitable_shell()
> This doesn't sound right either, because if CONFIG_SHELL is unset and
> SHELL is suitable, the code should use SHELL rather than trying to
> find one of our own.  (That is what the code does now.)

Well, it's not actually doing that at all.  The present code is just plain  
buggy.  It is making the assumption that SHELL _is_ the shell which is  
running the script; but as illustrated above, that is a bad assumption.  The  
existing code makes the erroneous conclusion that if SHELL supports  
functions, then the shell running the script supports functions.  This is a  
potentially bogus conclusion.  For instance, if my SHELL is set to  
/usr/local/bin/bash, yet the script is running via /bin/sh (on account of #!  
/bin/sh), then the present logic is to test /usr/local/bin/bash for function  
support and to erroneously apply the result of that check to the running  
shell (/bin/sh).  It's just all wrong.

> This won't print a diagnostic in the case where SHELL doesn't support
> shell functions and CONFIG_SHELL is set.  The code should print a
> diagnostic in that case, since the rest of the script will be executed
> with SHELL and it may not work because of the lack of support for
> shell functions.
> Option 2 has the same problem.

I don't think it is worth pursuing the issue of this particlular patch any  
further.  I have already prepared a much more comprehensive patch which  
addresses these problems, plus the problem of Autoconf CVS choosing a less  
functional shell over a more functional shell.  I will be submitting this new  
patch shortly.

-- ES