[Buildroot] [RFC PATCH 0/2] use `command -v' instead of `which'

[Petr Vorel]

Hi Yann, Arnout, all,

> Petr, Arnout, All,

> On 2021-09-30 22:16 +0200, Petr Vorel spake thusly:
> > > On 2021-09-26 23:32 +0200, Arnout Vandecappelle spake thusly:
> > > > On 21/09/2021 22:51, Petr Vorel wrote:
> > > This is causing quite some issues.
> [--SNIP--]
> > > Second, this is causing a lot of error messages:
> > >     $ make defconfig
> > >     [...]
> > >     $ make help
> > >     make[1]: command: Command not found
> > >     [...]
> > New error. But I was not able to reproduce it on x86_64 on current master
> > (5916cc5011). What am I missing to reproduce it?

> We've investigated with Arnout, who was also unable to reproduce, and we
> eventually found the cause for this issue, which is two fold.
Thanks a lot for investigation!

> First, make, at least in some versions of make, will run commands that
> it believes are "simple", directly with execve() (or any other exec*()
> wrapper), instead of running it through a shell via system().

> This is what happens when it see a command like this:

>     export PERL=$(shell command -v perl)
Hm, that's bad. Can you post any version which is affected?
It looks like I tested it only on 4.3, but have plenty of VM with older
make releases.

> which we have in package/Makefile.in at 240. In this case, make will try to
NOTE for myself (when debugging this later): package/Makefile.in (@240 is probably garbage)

> run the command (split à-la python), as can be seen with strace:

>     ['command', '-v', 'perl']

> Second, the issue was invisible to Arnout, because the distribution he
> uses, Fedora, provides /usr/bin/command, which is a simple shell script
> that just basically does:

>     #!/bin/sh
>     builtin command "${@}"

Hm, wrapping shell builtin into script is really strange.
BTW I was already thinking to add similar script to buildroot,
but I'm really surprised that Fedora added that to distro.

> However, this is provided by no package in the distribution I use,
> Ubuntu 20.04.1 LTS (filtering to ignore /usr/bin/commander et al.):

>     $ apt-file search bin/command |grep -E 'bin/command$'
>     [nothing]

This is probably Fedora/RHEL specific. I'll investigate which package it belongs
and ask Fedora maintainer for a reason (unfortunately Fedora does not have any
search like Debian https://packages.debian.org/ [1]).

> So, probably you did not see the error either, because your distribution
> also provides command as an actual executable. Could you check that by
> running:    which command     (Ahaha! :-])

> Note: if we change the line above to

>     export PERL=$(shell command -v perl 2>/dev/null)

> then make no longer believes this is a simple command, and will execute
> with system() and the warning goes away.
Hm, definitely worth to more investigate which make releases are affected and
report if not already fixed.

> So, bottom line, there are more impacts than previously expected, and we
> need to think the transition more carefully.
+1

> And to be extra clear: I am OK with transitionning away from which, or
> at least from relying on which being provided by the distro.
Thanks!

Also going back to the issue with bash 'command -v' implementation. First is it
also relevant to this issue or not?

You mentioned Ubuntu with bash as /bin/sh as a default shell in previous mail.
Testing 'make defconfig && make help 2>&1 |grep -i found' on bash as /bin/sh
does not trigger the error (make[1]: command: Command not found). Thus bash it's
not the problem (at least less problematic than make you mentioned).

Also, does anybody understand POSIX spec [2] whether only single command_name
can be used as Markus reported [3]? Should we report it to bash?

bash:
$ command -v ls uname
alias ls='ls --color=auto'
/usr/bin/uname

dash:
$ command -v ls uname
/usr/bin/ls

busybox sh
$ command -v ls uname
/usr/bin/ls

 I always used command -v with single argument in LTP and iputils, thus I
didn't get this problem.

[1] https://packages.debian.org/search?suite=default&section=all&arch=any&searchon=contents&keywords=%2Fusr%2Fbin%2Fcommand
[2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
[3] https://lore.kernel.org/buildroot/YVTIghzHs82uFBIe@pevik/T/#m95c17eb8374e4e3dd6eee700d397aa12cca0739e
[4] https://savannah.gnu.org/projects/which

> > > So, I suggest that we do revert this patch, and work on a better
> > > transition away from which, if at all. One very quick solution would be
> > > to bundle our own which in Buildroot and then we'd have a quick way out
> > > of that Debian's mess...
> > Sure, if it causes problems which are not easily fixed, I'm not against
> > reverting it. But I don't think that problem is that complex, that we'd need to
> > compile which. But I apologize for causing troubles.

> And again, I want to reiterate that: you have no reason to apologise. :-)
> Your patch was reviewed and applied, and there was no way we could have
> found the issues above without trying in the first place.
Yes, but next time I need testing on more distros, not just my laptop.

> But now, we can't keep this in the current state so, after discussing
> this with Arnout, I am going to revert the patch.
Sure, understand (I see you already reverted it).

> We can look at a better way to solve the Debian unstable issue about
> which, probably the first being to open a bug with them, so that they
> revert the warning, and second to find a way to no longer rely on which
> from the distro (either by transitionning to something else, or by
> bundling our own?).
I'll need to find more time to investigate the problem to suggest some
solution. I'd prefer to fix builtin shell than backport which (which is shell
just on Debian, there is also C version in upstream which discontinued [4]).


> Thanks for your work on Buildroot! :-)
yw :)

Kind regards,
Petr

> Regards,
> Yann E. MORIN.