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

["Yann E. MORIN" <yann.morin.1998@free.fr>]

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.

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)

which we have in package/Makefile.in@240. In this case, make will try to
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 "${@}"

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]

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.

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

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.

> > 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.

But now, we can't keep this in the current state so, after discussing
this with Arnout, I am going to revert the patch.

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?).

Thanks for your work on Buildroot! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot