RFR: 8243656: Shell built-in test in configure depends on help

[Magnus Ihse Bursie]

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
>