On 2020-05-06 10:52, Severin Gehwolf wrote:
On Wed, 2020-05-06 at 10:16 +0200, Magnus Ihse Bursie wrote:
On 2020-05-05 17:15, Severin Gehwolf wrote:
Hi,
Could I please get a review of this trivial change? Apparently using
the help builtin for determining whether or not a builtin is available
is not a good idea. A more portable way to do this is to use "command
-v" or "type". Thanks to Michael Zucchi for contributing this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8243656
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243656/02/webrev/
Hi Severin,
I missed commenting on this before you pushed it, but does this really
work?! Did you try it on your system?
I believe so.
I just tested the following:
$ command -V ps
ps is /bin/ps
$ command -V fg
fg is a shell builtin
$ command -v ps
/bin/ps
$ echo $?
0
$ command -v fg
fg
$ echo $?
0
That is, it does not seem like "command -v" is making any difference in
return value between builtins and external commands!
Sure, but is that a problem? The configure check is there as a fallback
if the tool is not found via AC_PATH_PROGS it tries this fallback (i.e.
it must be a built-in if "command -v <tool>" works, no?).
Aha. Sorry, I mis-read the code. I thought it was trying to make sure it
was a shell builtin, as in contrast to an external command.
I agree that the way the code is written it does solve the problem the
original author wanted solved, so it's working. However, it is somewhat
misleading. The comments state:
AC_MSG_NOTICE([Required tool $2 not found in PATH, checking built-in])
AC_MSG_NOTICE([Found $2 as shell built-in. Using it])
but this is technically not correct, since that is not what we're doing.
For instance, this test would also match a shell alias. (If this is
something we want to do, I don't know. I'd say "no" but I could imagine
someone arguing for the contrary. And the original "help <command>" did
not accept aliases, so it's sort of a regression...)
I'm not sure this is worth putting more energy in, but if you are up to
it, I think changing it to "type -t" would be slightly better. Then at
least the code will do what it actually claims to do. If it really
matters in any real world scenario, I don't know.
/Magnus
$ command -v help
help
$ echo $?
0
$ command -v foo
$ echo $?
1
Also, even if it should do, this is not documented and I don't think it
should be relied on -- as evident, it does not work on my system.
I'm confused, what's not documented? Everything you said seems to be
documented.
Also from 'man bash' for 'command' I see:
"""
If the -V or -v option is supplied, the exit status is 0 if command
was found, and 1 if not.
"""
In contrast, the (builtin) command "type" seems to work fine, and is
documented to work:
$ type -t ps
file
$ type -t fg
builtin
I recommend that use $(type -t) = builtin instead.
I agree type would work too. If you insist, I can change the fallback
from 'command -v' to 'type -t'.
Thoughts?
Thanks,
Severin
