mbox series

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

Message ID 20210921205112.39176-1-petr.vorel@gmail.com
Headers show
Series use `command -v' instead of `which' | expand

Message

Petr Vorel Sept. 21, 2021, 8:51 p.m. UTC
Hi,

I've tested the patchset on dash as the default shell. But it certainly
deserve more people to have look and test.

NOTE: If accepted, we might want to use `command -v' also in scripts:

$ git grep '$(which'
package/fakedate/fakedate:for date in $(which -a date |tac); do
package/transmission/S92transmission:DAEMON=$(which $NAME)

Kind regards,
Petr

Petr Vorel (2):
  make: support: use `command -v' instead of `which'
  support/dependencies: don't check for `which'

 Makefile                                      | 22 +++++++++----------
 package/Makefile.in                           |  8 +++----
 support/dependencies/check-host-bison-flex.mk |  4 ++--
 support/dependencies/check-host-cmake.sh      |  2 +-
 support/dependencies/check-host-gzip.sh       |  2 +-
 support/dependencies/check-host-lzip.sh       |  4 ++--
 support/dependencies/check-host-python3.sh    |  2 +-
 support/dependencies/check-host-tar.sh        |  4 ++--
 support/dependencies/check-host-xzcat.sh      |  4 ++--
 support/dependencies/dependencies.sh          | 18 +++++++--------
 .../pkg-toolchain-external.mk                 |  2 +-
 11 files changed, 35 insertions(+), 37 deletions(-)

Comments

Yann E. MORIN Sept. 30, 2021, 8:04 p.m. UTC | #1
Petr, Arnout, All,

On 2021-09-26 23:32 +0200, Arnout Vandecappelle spake thusly:
> On 21/09/2021 22:51, Petr Vorel wrote:
> >I've tested the patchset on dash as the default shell. But it certainly
> >deserve more people to have look and test.
> 
>  Well, as the commit message says: it's POSIX so it should be supported by
> everything. which has a much smaller chance of being supported.

This is causing quite some issues.

First, 'command -v' does not behave the same way 'which' used to, when
passed more than one parameter, because some shells are not compliant to
POSIX (this might be a bug, but nonetheless it affects the most widely
used shell out there, bash) [0].

Second, this is causing a lot of error messages:

    $ make defconfig
    [...]
    $ make help
    make[1]: command: Command not found
    [...]

The original commit reports that 'which' is broken in Debian, but I was
not able to reproduce in Bullseye, where 'which' still works as expected
and does not emit any extra warning.

So, we are trying to fix something that is broken on a development
version of Debian, but that still works in all known released
distributions.

I usually am quite in favour of sticking to POSIX tools, but which has
been ubiquitous in the past 30 years or so, and I would consider that
Debian's which *is* broken for reporting such deprecation warnings.

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

Anyway, I'd vote "revert".

Regards,
Yann E. MORIN.

[0] https://lore.kernel.org/buildroot/YVTIghzHs82uFBIe@pevik/T/#m95c17eb8374e4e3dd6eee700d397aa12cca0739e
Petr Vorel Sept. 30, 2021, 8:16 p.m. UTC | #2
Hi Yann, all,

> Petr, Arnout, All,

> On 2021-09-26 23:32 +0200, Arnout Vandecappelle spake thusly:
> > On 21/09/2021 22:51, Petr Vorel wrote:
> > >I've tested the patchset on dash as the default shell. But it certainly
> > >deserve more people to have look and test.

> >  Well, as the commit message says: it's POSIX so it should be supported by
> > everything. which has a much smaller chance of being supported.

> This is causing quite some issues.

> First, 'command -v' does not behave the same way 'which' used to, when
> passed more than one parameter, because some shells are not compliant to
> POSIX (this might be a bug, but nonetheless it affects the most widely
> used shell out there, bash) [0].
Yes, there has been a proposal, how to fix this.

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

> The original commit reports that 'which' is broken in Debian, but I was
> not able to reproduce in Bullseye, where 'which' still works as expected
> and does not emit any extra warning.
Yes, the waring is in debianutils 5.x, which is still in Debian unstable (not
even in the testing) [1].

> So, we are trying to fix something that is broken on a development
> version of Debian, but that still works in all known released
> distributions.
'command -v' and 'type' has been here quite long time as well.

> I usually am quite in favour of sticking to POSIX tools, but which has
> been ubiquitous in the past 30 years or so, and I would consider that
> Debian's which *is* broken for reporting such deprecation warnings.

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

Kind regards,
Petr

> Anyway, I'd vote "revert".

> Regards,
> Yann E. MORIN.

> [0] https://lore.kernel.org/buildroot/YVTIghzHs82uFBIe@pevik/T/#m95c17eb8374e4e3dd6eee700d397aa12cca0739e
[1] https://tracker.debian.org/pkg/debianutils
Yann E. MORIN Sept. 30, 2021, 8:41 p.m. UTC | #3
Petr, All,

On 2021-09-30 22:16 +0200, Petr Vorel spake thusly:
> > Petr, Arnout, All,
> > On 2021-09-26 23:32 +0200, Arnout Vandecappelle spake thusly:
> > > On 21/09/2021 22:51, Petr Vorel wrote:
> > > >I've tested the patchset on dash as the default shell. But it certainly
> > > >deserve more people to have look and test.
> > >  Well, as the commit message says: it's POSIX so it should be supported by
> > > everything. which has a much smaller chance of being supported.
> > This is causing quite some issues.
> 
> > First, 'command -v' does not behave the same way 'which' used to, when
> > passed more than one parameter, because some shells are not compliant to
> > POSIX (this might be a bug, but nonetheless it affects the most widely
> > used shell out there, bash) [0].
> Yes, there has been a proposal, how to fix this.

The problem I see with that proposal, is that it catters for a few
variables only. We might not even notice until much later that something
else is borken in some weird ways... And we were lucky that Markus did
the investigation and was able to find the actual root cause.

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

I'm on a bog-down standard Ubuntu 20.04, prety up-to-date. My /bin/sh is
bash.

> > The original commit reports that 'which' is broken in Debian, but I was
> > not able to reproduce in Bullseye, where 'which' still works as expected
> > and does not emit any extra warning.
> Yes, the waring is in debianutils 5.x, which is still in Debian unstable (not
> even in the testing) [1].
> 
> > So, we are trying to fix something that is broken on a development
> > version of Debian, but that still works in all known released
> > distributions.
> 'command -v' and 'type' has been here quite long time as well.

I am not denying this. I'm just saying that we are trading a working
situation in all released distributions and a failure in Debian
unstable, for a failure in a quite a lot, in not most, of the previously
working cases.

> > I usually am quite in favour of sticking to POSIX tools, but which has
> > been ubiquitous in the past 30 years or so, and I would consider that
> > Debian's which *is* broken for reporting such deprecation warnings.
> 
> > 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.

which is a shell script in Debian; we need not compile it at all. Let's
just grab the latest working which that did not have that warning:

    $ file /usr/bin/which
    /usr/bin/which: POSIX shell script, ASCII text executable

> But I apologize for causing troubles.

Do not apologise, this is not needed. The trouble (as you say) is
standard development issues that will eventually be resolved, one way or
another, no worries. ;-)

Thank you! :-)

Regards,
Yann E. MORIN.

> Kind regards,
> Petr
> 
> > Anyway, I'd vote "revert".
> 
> > Regards,
> > Yann E. MORIN.
> 
> > [0] https://lore.kernel.org/buildroot/YVTIghzHs82uFBIe@pevik/T/#m95c17eb8374e4e3dd6eee700d397aa12cca0739e
> [1] https://tracker.debian.org/pkg/debianutils
Yann E. MORIN Oct. 1, 2021, 6:03 p.m. UTC | #4
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.
Petr Vorel Oct. 2, 2021, 7:22 p.m. UTC | #5
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@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.
Arnout Vandecappelle Oct. 3, 2021, 9:49 a.m. UTC | #6
On 02/10/2021 21:22, Petr Vorel wrote:
> 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@240. In this case, make will try to
> NOTE for myself (when debugging this later): package/Makefile.in (@240 is probably garbage)

  It *is* on line 240 in current master, and package/Makefile.in hasn't changed 
since July 4 (except for the command -v replacement, of course).

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

  POSIX doesn't say anything about `command` etc. being shell builtins. Thus, a 
program written for POSIX may assume that it can execvp("command", ...) (i.e. 
without going through the shell).

  I assume that someone in RedHat encountered such an issue and that that was 
their solution.

> BTW I was already thinking to add similar script to buildroot,

  Right - instead of providing our on `which`, we can also provide our own 
`command`!

  One small caveat though: we currently don't add anything to PATH. Adding our 
own thing (whether it's which or command) will require us to add something to 
PATH, which by itself can have surprising effects as well.

> 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

  Yes it is. It's provided by the bash package.

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

  However, a future version of GNU make may very well decide that '2>/dev/null' 
is something it can handle itself, and bypass the shell as well.

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

  It's an issue if we pass several parameters to `command -v`. We probably 
should simply never do that - the case that Markus found is a bug anyway.


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

  The POSIX spec says:

command [-p][-v|-V] command_name

So it's only specified with a single argument. The POSIX spec is pretty vague 
about what to do with stuff that is not specified, like a second argument. So I 
guess implementations can do what they want...


  By the way, the differences between which and command -v are a bit larger than 
this. command -v also works for shell builtins, while which doesn't - unless 
which is defined as a function, which is the case for the `which` package in 
Fedora...

$ command -v ls
alias ls='ls -AFh'
$ which ls
alias ls='ls -AFh'
         /usr/bin/ls
$ /bin/which ls
/usr/bin/ls
$ type which
which is a function
which ()
{
     ( alias;
     eval ${which_declare} ) | /usr/bin/which --tty-only --read-alias 
--read-functions --show-tilde --show-dot "$@"
}


  I think we can safely say: this is a mess :-)


  From this, I actually conclude that it is indeed safer to go to command -v, 
with a wrapper script that we add to $PATH to handle the case where make tries 
to bypass the shell.


  Regards,
  Arnout


> 
> 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.
Petr Vorel Oct. 3, 2021, 6:05 p.m. UTC | #7
Hi Arnout, Yann, all,

<snip>

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

>  It *is* on line 240 in current master, and package/Makefile.in hasn't
> changed since July 4 (except for the command -v replacement, of course).

+1, thanks.

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

>  POSIX doesn't say anything about `command` etc. being shell builtins. Thus,
> a program written for POSIX may assume that it can execvp("command", ...)
> (i.e. without going through the shell).
Good point. Obviously POSIX didn't realize problems we encountered. IMHO it'd be
great to specify this precisely in POSIX.

>  I assume that someone in RedHat encountered such an issue and that that was
> their solution.
I wonder if they tried to contribute it to coreutils. It'd be great to have
upstream solution.

> > BTW I was already thinking to add similar script to buildroot,

>  Right - instead of providing our on `which`, we can also provide our own
> `command`!
Yes (fairly smaller amount of code, which likely solve all our problems).
I'll also test "builtin command" in Makefile (I suppose it will be the same:
"Command not found").

>  One small caveat though: we currently don't add anything to PATH. Adding
> our own thing (whether it's which or command) will require us to add
> something to PATH, which by itself can have surprising effects as well.
Good point. IMHO we'll have to go this way (and solve any problem with PATH)
even if we wan to take 'which'. Sooner or later which version disappear from
Debian. GNU which is discontinued, but I suppose distros which ship it now (RHEL/Fedora and SLES/Tumbleweed and probably many others) will keep it for long time.

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

>  Yes it is. It's provided by the bash package.
+1

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

>  However, a future version of GNU make may very well decide that
> '2>/dev/null' is something it can handle itself, and bypass the shell as
> well.
Maybe it'd be good to teach make always run 'command' via system().

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

>  It's an issue if we pass several parameters to `command -v`. We probably
> should simply never do that - the case that Markus found is a bug anyway.
+1

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

>  The POSIX spec says:

> command [-p][-v|-V] command_name

> So it's only specified with a single argument. The POSIX spec is pretty
Yep, I understood it that way.
> vague about what to do with stuff that is not specified, like a second
> argument. So I guess implementations can do what they want...
+1

>  By the way, the differences between which and command -v are a bit larger
> than this. command -v also works for shell builtins, while which doesn't -
> unless which is defined as a function, which is the case for the `which`
> package in Fedora...

> $ command -v ls
> alias ls='ls -AFh'
> $ which ls
> alias ls='ls -AFh'
>         /usr/bin/ls
> $ /bin/which ls
> /usr/bin/ls
> $ type which
> which is a function
> which ()
> {
>     ( alias;
>     eval ${which_declare} ) | /usr/bin/which --tty-only --read-alias
> --read-functions --show-tilde --show-dot "$@"
> }

Yes, 'type' is quite different than which/command -v. More verbose, thus I
understand why 'type' is POSIX extension and 'command -v' mandatory.

>  I think we can safely say: this is a mess :-)


>  From this, I actually conclude that it is indeed safer to go to command -v,
> with a wrapper script that we add to $PATH to handle the case where make
> tries to bypass the shell.
+1. I'll try to have look into it.

Thanks a lot to all for your comments.

Kind regards,
Petr

>  Regards,
>  Arnout

<snip>
Peter Korsgaard Oct. 9, 2021, 10:01 a.m. UTC | #8
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  From this, I actually conclude that it is indeed safer to go to
 > command -v, with a wrapper script that we add to $PATH to handle the
 > case where make tries to bypass the shell.

If command is always a shell builtin and we already do the dance to find
bash, how about just doing something like:

$(shell $(SHELL) -c "command -v ...")?
Petr Vorel Oct. 10, 2021, 9:12 p.m. UTC | #9
Hi all,

> >>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

> Hi,

>  >  From this, I actually conclude that it is indeed safer to go to
>  > command -v, with a wrapper script that we add to $PATH to handle the
>  > case where make tries to bypass the shell.

> If command is always a shell builtin and we already do the dance to find
> bash, how about just doing something like:

> $(shell $(SHELL) -c "command -v ...")?
Peter, good point, thanks! That would safe us to modify path.
This works on make 4.3 on Debian with /bin/sh, I'll try to test it on more VMs
(+ find one where plain $(shell command -v ...) does not work).

Kind regards,
Petr