[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: |
Tue, 6 Jan 2004 19:10:54 -0500 |
On 06 Jan 2004 12:34:42 -0800, Paul Eggert wrote:
> Eric Sunshine <address@hidden> writes:
> > In the case when $SHELL does not support functions,
> > AS_SHELL_SANITIZE() reports that it could not find an appropriate
> > shell even if it actually locates such a shell (which it assigns to
> > CONFIG_SHELL).
> Hmm, how could it assign to CONFIG_SHELL and then report that it
> couldn't find a shell? The code does this:
> CONFIG_SHELL=$as_dir/$as_base
> export CONFIG_SHELL
> exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
> so if it assigns to CONFIG_SHELL, it execs $CONFIG_SHELL, which means
> it shouldn't report that it can't find a shell.
Indeed, that's the way it is supposed to work, but that's not the way it is
presently coded up. If you trace through it, you will see the flaw. Here is
what the code is actually doing (pseudo-code):
if $SHELL supports functions
# Yay!
else
case $CONFIG_SHELL in
'')
CONFIG_SHELL=try_to_find_suitable_shell()
if CONFIG_SHELL is set
exec $CONFIG_SHELL $this_script
endif
;;
*)
echo "Failed to find suitable shell" <<-- WRONG!
;;
esac
endif
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 which incorrectly
prints an error saying that the shell was not found (even though it was).
One way to correct the logic is as follows:
case $CONFIG_SHELL in
CONFIG_SHELL=try_to_find_suitable_shell()
if CONFIG_SHELL is set
exec $CONFIG_SHELL $this_script
else
echo "Failed to find suitable shell" <<-- CORRECT
endif
;;
*)
;;
esac
This way, we emit the error message iff if we actually fail to find a
suitable shell.
> Under the proposed patch, 'configure' would sometimes ignore
> CONFIG_SHELL and substitute its own CONFIG_SHELL. This doesn't seem
> right to me, as the user shouldn't be second-guessed.
I also came to this conclusion after sending out the patch and decided to
send a simpler patch which still allows the user to shoot himself in the foot
if he so desires.
> Yes, this looks like a problem to me too. How about this patch instead?
No, that patch does not work either. It also incorrectly complains that it
could not find a shell even when it does find one. I have included below two
separate super simple patches, each of which corrects the logic. The second
of the two patches is lengthier, but it is also much simpler to understand
since it doesn't suffer from the convolusions which gave rise to this bug in
the first place. Choose the patch which you like best.
> (I don't know what the "In the future" comment means, so I removed it.)
I presume that Paolo could explain it since he wrote it.
Eric
2004-01-06 Eric Sunshine <address@hidden>
* lib/m4sugar/m4sh.m4 (AS_SHELL_SANITIZE): Fixed bogus error reporting
logic. If $SHELL did not support shell functions, and if it found a
shell which did support functions, it would complain that it failed to
find such a shell (backward logic). Worse, it did not complain if it
failed to find a suitable shell. Now it complains iff it fails to find
a function-supporting shell.
>>> option 1 <<<
--- lib/m4sugar/m4sh.m4 Tue Jan 6 18:39:20 2004
+++ lib/m4sugar/m4sh.m4-fix Tue Jan 6 18:37:32 2004
@@ -264,9 +264,11 @@
exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
]);;
esac
- done]);;
- *)
+ done])
+ # If we got this far, we failed to find a function-supporting shell.
$1;;
+ *)
+ ;;
esac
])
>>> option 1 <<<
--- lib/m4sugar/m4sh.m4 Tue Jan 6 19:04:41 2004
+++ lib/m4sugar/m4sh.m4-fix2 Tue Jan 6 19:04:26 2004
@@ -249,9 +249,8 @@
fi
dnl In the future, the `else' branch will be that in AS_INIT_WITH_SHELL_FN.
-AS_IF([_AS_SHELL_FN_WORK([$SHELL])], [], [
- case $CONFIG_SHELL in
- '')
+AS_IF([test -z "$CONFIG_SHELL"],
+ [AS_IF([_AS_SHELL_FN_WORK([$SHELL])], [], [
_AS_PATH_WALK([/bin$PATH_SEPARATOR/usr/bin$PATH_SEPARATOR$PATH],
[for as_base in sh bash ksh sh5; do
case $as_dir in
@@ -264,11 +263,9 @@
exec "$CONFIG_SHELL" "$as_myself" ${1+"address@hidden"}
]);;
esac
- done]);;
- *)
- $1;;
- esac
-])
+ done])
+ $1
+])])
# Work around bugs in pre-3.0 UWIN ksh.
$as_unset ENV MAIL MAILPATH