[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Shell function reporting
From: |
Eric Sunshine |
Subject: |
Re: [PATCH] Shell function reporting |
Date: |
Wed, 7 Jan 2004 13:47:48 -0500 |
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