Patchwork [1/1] Prefer 'command -v' over 'which' (for portability)

login
register
mail settings
Submitter Bjørn Forsman
Date Jan. 25, 2014, 2:11 p.m.
Message ID <1390659116-18865-1-git-send-email-bjorn.forsman@gmail.com>
Download mbox | patch
Permalink /patch/314144/
State Rejected
Headers show

Comments

Bjørn Forsman - Jan. 25, 2014, 2:11 p.m.
'command -v' is defined by POSIX (and is available "everywhere"),
'which' is not.

This is improves user experience when 'which' isn't installed so that
there won't be error messages about missing 'which' *before* the
Buildroot dependency check is run.

Current behaviour:

  $ make
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  /bin/sh: which: command not found
  support/dependencies/check-host-tar.sh: line 5: which: command not found
  support/dependencies/check-host-tar.sh: line 7: which: command not found
  /home/bfo/buildroot/support/dependencies/dependencies.sh: line 56:
  which: command not found

  You must install 'which' on your build machine
  make: *** [core-dependencies] Error 1

New behaviour:

  $ make

  You must install 'which' on your build machine
  make: *** [core-dependencies] Error 1

Signed-off-by: Bjørn Forsman <bjorn.forsman@gmail.com>
---
 Makefile                               | 22 +++++++++++-----------
 package/Makefile.in                    | 10 +++++-----
 support/dependencies/check-host-tar.sh |  4 ++--
 support/dependencies/dependencies.sh   | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 28 deletions(-)
Thomas Petazzoni - Jan. 27, 2014, 6:44 a.m.
Dear Bjørn Forsman,

On Sat, 25 Jan 2014 15:11:56 +0100, Bjørn Forsman wrote:

>   $ make
> 
>   You must install 'which' on your build machine
>   make: *** [core-dependencies] Error 1

But then, do we still need to have "which" installed? Maybe this hard
dependency is no longer necessary?

Thomas
Arnout Vandecappelle - Jan. 27, 2014, 7:20 a.m.
On 25/01/14 15:11, Bjørn Forsman wrote:
> 'command -v' is defined by POSIX (and is available "everywhere"),
> 'which' is not.
>
> This is improves user experience when 'which' isn't installed so that
> there won't be error messages about missing 'which'*before*  the
> Buildroot dependency check is run.
>
> Current behaviour:
>
>    $ make
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    /bin/sh: which: command not found
>    support/dependencies/check-host-tar.sh: line 5: which: command not found
>    support/dependencies/check-host-tar.sh: line 7: which: command not found
>    /home/bfo/buildroot/support/dependencies/dependencies.sh: line 56:
>    which: command not found
>
>    You must install 'which' on your build machine
>    make: *** [core-dependencies] Error 1
>
> New behaviour:
>
>    $ make
>
>    You must install 'which' on your build machine
>    make: *** [core-dependencies] Error 1
>
> Signed-off-by: Bjørn Forsman<bjorn.forsman@gmail.com>

  There are also instances of which in manual.mk and toolchain-external.mk.

  With those removed, I wonder if it is even necessary to have which 
installed at all? Could you test that with a largish config?

  Regards,
  Arnout
Samuel Martin - Jan. 27, 2014, 9:19 a.m.
Bjørn, all,


2014-01-25 Bjørn Forsman <bjorn.forsman@gmail.com>

> 'command -v' is defined by POSIX (and is available "everywhere"),
> 'which' is not.
>
> This is improves user experience when 'which' isn't installed so that
> there won't be error messages about missing 'which' *before* the
> Buildroot dependency check is run.
>
> Current behaviour:
>
>   $ make
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   /bin/sh: which: command not found
>   support/dependencies/check-host-tar.sh: line 5: which: command not found
>   support/dependencies/check-host-tar.sh: line 7: which: command not found
>   /home/bfo/buildroot/support/dependencies/dependencies.sh: line 56:
>   which: command not found
>
>   You must install 'which' on your build machine
>   make: *** [core-dependencies] Error 1
>

"command -v" seems not playing very well with aliases:

$ command -v make
alias make='~/.config/ctafconf/bin/colorwarper make'
$ which make
/usr/bin/make


Regards,
Bjørn Forsman - Jan. 27, 2014, 7:47 p.m.
On 27 January 2014 07:44, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Bjørn Forsman,
>
> On Sat, 25 Jan 2014 15:11:56 +0100, Bjørn Forsman wrote:
>
>>   $ make
>>
>>   You must install 'which' on your build machine
>>   make: *** [core-dependencies] Error 1
>
> But then, do we still need to have "which" installed? Maybe this hard
> dependency is no longer necessary?

Don't know. I was under the impression that "which" was used by
autotools, but I didn't check it.
Bjørn Forsman - Jan. 27, 2014, 8:01 p.m.
On 27 January 2014 10:19, Samuel Martin <s.martin49@gmail.com> wrote:
> Bjørn, all,

[...]

> "command -v" seems not playing very well with aliases:
>
> $ command -v make
> alias make='~/.config/ctafconf/bin/colorwarper make'
> $ which make
> /usr/bin/make

Ah, correct.

There is "type -P" which is more like "which" in that regard, but the
"-P" option doesn't seem to be specified by POSIX. For example, "type
-P which" fails in dash.

Instead of an alias, you could write a one-liner shell wrapper? I tend
to prefer wrappers over aliases anyway, because of issues like these;
aliases sometimes behave differently than proper commands.

Best reagards,
Bjørn Forsman
Bjørn Forsman - Jan. 27, 2014, 8:13 p.m.
On 27 January 2014 08:20, Arnout Vandecappelle <arnout@mind.be> wrote:
[...]
>  There are also instances of which in manual.mk and toolchain-external.mk.

Oh, I missed those. Thanks.

>  With those removed, I wonder if it is even necessary to have which
> installed at all? Could you test that with a largish config?

Testing a "raspberrypi_defconfig" build now without any "which"
(removed the check for "which" check in dependencies.sh too).
Thomas De Schampheleire - Jan. 27, 2014, 8:32 p.m.
On Mon, Jan 27, 2014 at 9:01 PM, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
> On 27 January 2014 10:19, Samuel Martin <s.martin49@gmail.com> wrote:
>> Bjørn, all,
>
> [...]
>
>> "command -v" seems not playing very well with aliases:
>>
>> $ command -v make
>> alias make='~/.config/ctafconf/bin/colorwarper make'
>> $ which make
>> /usr/bin/make
>
> Ah, correct.
>
> There is "type -P" which is more like "which" in that regard, but the
> "-P" option doesn't seem to be specified by POSIX. For example, "type
> -P which" fails in dash.
>
> Instead of an alias, you could write a one-liner shell wrapper? I tend
> to prefer wrappers over aliases anyway, because of issues like these;
> aliases sometimes behave differently than proper commands.

Why I also think this is saner than aliases, we cannot dictate this
principle to all buildroot users, so buildroot cannot rely on aliases
not being used...

Best regards,
Thomas
Bjørn Forsman - Jan. 27, 2014, 9:09 p.m.
On 27 January 2014 21:32, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> On Mon, Jan 27, 2014 at 9:01 PM, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
>> On 27 January 2014 10:19, Samuel Martin <s.martin49@gmail.com> wrote:
>>> Bjørn, all,
>>
>> [...]
>>
>>> "command -v" seems not playing very well with aliases:
>>>
>>> $ command -v make
>>> alias make='~/.config/ctafconf/bin/colorwarper make'
>>> $ which make
>>> /usr/bin/make
>>
>> Ah, correct.
>>
>> There is "type -P" which is more like "which" in that regard, but the
>> "-P" option doesn't seem to be specified by POSIX. For example, "type
>> -P which" fails in dash.
>>
>> Instead of an alias, you could write a one-liner shell wrapper? I tend
>> to prefer wrappers over aliases anyway, because of issues like these;
>> aliases sometimes behave differently than proper commands.
>
> Why I also think this is saner than aliases, we cannot dictate this
> principle to all buildroot users, so buildroot cannot rely on aliases
> not being used...

In the example above, "which" finds the /usr/bin/make binary and
ignores the alias. So what do you propose that Buildroot should
support?

The way I see it we have three options:

1) Allow aliases but silently ignore them (current behaviour)
2) Allow aliases and handle them properly (requires some changes)
3) Disallow aliases (and suggest wrappers?)

If we apply this s/which/command -v/ patch I think we can still obtain
the current behaviour by just running "unalias" for each core
dependency.

Best regards,
Bjørn Forsman
Bjørn Forsman - Jan. 27, 2014, 10:23 p.m.
On 27 January 2014 21:13, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
> On 27 January 2014 08:20, Arnout Vandecappelle <arnout@mind.be> wrote:
> [...]
>>  There are also instances of which in manual.mk and toolchain-external.mk.
>
> Oh, I missed those. Thanks.
>
>>  With those removed, I wonder if it is even necessary to have which
>> installed at all? Could you test that with a largish config?
>
> Testing a "raspberrypi_defconfig" build now without any "which"
> (removed the check for "which" check in dependencies.sh too).

"make raspberrypi_defconfig && make" works (build completed) with no
"which" at all.
Arnout Vandecappelle - Jan. 28, 2014, 6:54 a.m.
On 27/01/14 10:19, Samuel Martin wrote:
> "command -v" seems not playing very well with aliases:
>
> $ command -v make
> alias make='~/.config/ctafconf/bin/colorwarper make'
> $ which make
> /usr/bin/make

  I don't think your .profile gets executed when running a shell script, 
so the alias will not be set. Bash does use BASH_ENV to specify a startup 
script, so perhaps we should unexport that.

  Regards,
  Arnout
Yann E. MORIN - Feb. 15, 2014, 11:44 a.m.
Arnout, All,

Jumping back into this old thread, after it was referenced by Maxime in
his XBMC series...

On 2014-01-28 07:54 +0100, Arnout Vandecappelle spake thusly:
> On 27/01/14 10:19, Samuel Martin wrote:
> >"command -v" seems not playing very well with aliases:
> >
> >$ command -v make
> >alias make='~/.config/ctafconf/bin/colorwarper make'
> >$ which make
> >/usr/bin/make
> 
>  I don't think your .profile gets executed when running a shell script, so
> the alias will not be set. Bash does use BASH_ENV to specify a startup
> script, so perhaps we should unexport that.

Here is a little test I did:

    $ cat Makefile
    all:
        echo "'$${buz}'"
        foo

    $ cat /tmp/foo.env
    buz="BUZ"
    echo BAR
    alias foo='echo FOO'

    $ BASH_ENV=/tmp/foo.env ENV=/tmp/foo.env make
    echo "'${buz}'"
    ''
    foo
    make: foo: Command not found
    make: *** [all] Error 127

So, it looks like neither BASH_ENV nor ENV are parsed (ENV is for when
bash is invoked in POSIX mode).

Regards,
Yann E. MORIN.
Bjørn Forsman - Feb. 15, 2014, 1:29 p.m.
On 15 February 2014 12:44, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Arnout, All,
>
> Jumping back into this old thread, after it was referenced by Maxime in
> his XBMC series...
>
> On 2014-01-28 07:54 +0100, Arnout Vandecappelle spake thusly:
>> On 27/01/14 10:19, Samuel Martin wrote:
>> >"command -v" seems not playing very well with aliases:
>> >
>> >$ command -v make
>> >alias make='~/.config/ctafconf/bin/colorwarper make'
>> >$ which make
>> >/usr/bin/make
>>
>>  I don't think your .profile gets executed when running a shell script, so
>> the alias will not be set. Bash does use BASH_ENV to specify a startup
>> script, so perhaps we should unexport that.
>
> Here is a little test I did:
>
>     $ cat Makefile
>     all:
>         echo "'$${buz}'"
>         foo
>
>     $ cat /tmp/foo.env
>     buz="BUZ"
>     echo BAR
>     alias foo='echo FOO'
>
>     $ BASH_ENV=/tmp/foo.env ENV=/tmp/foo.env make
>     echo "'${buz}'"
>     ''
>     foo
>     make: foo: Command not found
>     make: *** [all] Error 127
>
> So, it looks like neither BASH_ENV nor ENV are parsed (ENV is for when
> bash is invoked in POSIX mode).

I just want to mention an interesting corner case in handing of
$(shell ...) in gnu make, something I just now became aware of.

The (weird) thing is that this works:

$(shell command -v sed || echo sed)    # WORKS

while this does not:

$(shell command -v sed)    # FAILS

The reason for this behaviour (from stackoverflow,
http://stackoverflow.com/questions/12989869/calling-command-v-find-from-gnu-makefile):

----
Judging from a quick look at job.c in GNU make's sources, it attempts
to avoid launching a shell when it can, i.e. when the command line is
simple enough (of the form cmd args, without redirection, compound
commands, etc.) and the shell is the default one. The issue is then
that command is a built-in and does not have an associated executable,
hence the error message from make. It does not occur when you have >
/dev/null as make considers the command as too complicated and leaves
it tosh to launch it.
----

So, is this OK for merge now?

Best regards,
Bjørn Forsman

Patch

diff --git a/Makefile b/Makefile
index 9dfb1e0..e06bfe1 100644
--- a/Makefile
+++ b/Makefile
@@ -192,12 +192,12 @@  HOSTAS:=as
 endif
 ifndef HOSTCC
 HOSTCC:=gcc
-HOSTCC:=$(shell which $(HOSTCC) || type -p $(HOSTCC) || echo gcc)
+HOSTCC:=$(shell command -v $(HOSTCC) || type -p $(HOSTCC) || echo gcc)
 endif
 HOSTCC_NOCCACHE:=$(HOSTCC)
 ifndef HOSTCXX
 HOSTCXX:=g++
-HOSTCXX:=$(shell which $(HOSTCXX) || type -p $(HOSTCXX) || echo g++)
+HOSTCXX:=$(shell command -v $(HOSTCXX) || type -p $(HOSTCXX) || echo g++)
 endif
 HOSTCXX_NOCCACHE:=$(HOSTCXX)
 ifndef HOSTFC
@@ -221,15 +221,15 @@  endif
 ifndef HOSTRANLIB
 HOSTRANLIB:=ranlib
 endif
-HOSTAR:=$(shell which $(HOSTAR) || type -p $(HOSTAR) || echo ar)
-HOSTAS:=$(shell which $(HOSTAS) || type -p $(HOSTAS) || echo as)
-HOSTFC:=$(shell which $(HOSTLD) || type -p $(HOSTLD) || echo || which g77 || type -p g77 || echo gfortran)
-HOSTCPP:=$(shell which $(HOSTCPP) || type -p $(HOSTCPP) || echo cpp)
-HOSTLD:=$(shell which $(HOSTLD) || type -p $(HOSTLD) || echo ld)
-HOSTLN:=$(shell which $(HOSTLN) || type -p $(HOSTLN) || echo ln)
-HOSTNM:=$(shell which $(HOSTNM) || type -p $(HOSTNM) || echo nm)
-HOSTOBJCOPY:=$(shell which $(HOSTOBJCOPY) || type -p $(HOSTOBJCOPY) || echo objcopy)
-HOSTRANLIB:=$(shell which $(HOSTRANLIB) || type -p $(HOSTRANLIB) || echo ranlib)
+HOSTAR:=$(shell command -v $(HOSTAR) || type -p $(HOSTAR) || echo ar)
+HOSTAS:=$(shell command -v $(HOSTAS) || type -p $(HOSTAS) || echo as)
+HOSTFC:=$(shell command -v $(HOSTLD) || type -p $(HOSTLD) || echo || command -v g77 || type -p g77 || echo gfortran)
+HOSTCPP:=$(shell command -v $(HOSTCPP) || type -p $(HOSTCPP) || echo cpp)
+HOSTLD:=$(shell command -v $(HOSTLD) || type -p $(HOSTLD) || echo ld)
+HOSTLN:=$(shell command -v $(HOSTLN) || type -p $(HOSTLN) || echo ln)
+HOSTNM:=$(shell command -v $(HOSTNM) || type -p $(HOSTNM) || echo nm)
+HOSTOBJCOPY:=$(shell command -v $(HOSTOBJCOPY) || type -p $(HOSTOBJCOPY) || echo objcopy)
+HOSTRANLIB:=$(shell command -v $(HOSTRANLIB) || type -p $(HOSTRANLIB) || echo ranlib)
 
 export HOSTAR HOSTAS HOSTCC HOSTCXX HOSTFC HOSTLD
 export HOSTCC_NOCCACHE HOSTCXX_NOCCACHE
diff --git a/package/Makefile.in b/package/Makefile.in
index 2e433fd..115049a 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -4,7 +4,7 @@  endif
 ifndef HOSTMAKE
 HOSTMAKE=$(MAKE)
 endif
-HOSTMAKE :=$(shell which $(HOSTMAKE) || type -p $(HOSTMAKE) || echo make)
+HOSTMAKE :=$(shell command -v $(HOSTMAKE) || type -p $(HOSTMAKE) || echo make)
 
 # If BR2_LEVEL is 0, scale the maximum concurrency with the number of
 # CPUs. An additional job is used in order to keep processors busy
@@ -190,10 +190,10 @@  TARGET_STRIP=true
 STRIPCMD=$(TARGET_STRIP)
 KSTRIPCMD=$(TARGET_STRIP)
 endif
-INSTALL:=$(shell which install || type -p install)
-FLEX:=$(shell which flex || type -p flex)
-BISON:=$(shell which bison || type -p bison)
-SED:=$(shell which sed || type -p sed) -i -e
+INSTALL:=$(shell command -v install || type -p install)
+FLEX:=$(shell command -v flex || type -p flex)
+BISON:=$(shell command -v bison || type -p bison)
+SED:=$(shell command -v sed || type -p sed) -i -e
 
 HOST_CPPFLAGS  = -I$(HOST_DIR)/usr/include
 HOST_CFLAGS   ?= -O2
diff --git a/support/dependencies/check-host-tar.sh b/support/dependencies/check-host-tar.sh
index 2cfc2b3..a406bd5 100755
--- a/support/dependencies/check-host-tar.sh
+++ b/support/dependencies/check-host-tar.sh
@@ -2,9 +2,9 @@ 
 
 candidate="$1"
 
-tar=`which $candidate`
+tar=`command -v $candidate`
 if [ ! -x "$tar" ]; then
-	tar=`which tar`
+	tar=`command -v tar`
 	if [ ! -x "$tar" ]; then
 		# echo nothing: no suitable tar found
 		exit 1
diff --git a/support/dependencies/dependencies.sh b/support/dependencies/dependencies.sh
index 47d4d10..335c976 100755
--- a/support/dependencies/dependencies.sh
+++ b/support/dependencies/dependencies.sh
@@ -53,20 +53,20 @@  if test -n "$PERL_MM_OPT" ; then
 fi
 
 # Verify that which is installed
-if ! which which > /dev/null ; then
+if ! command -v which > /dev/null ; then
 	echo
 	echo "You must install 'which' on your build machine";
 	exit 1;
 fi;
 
-if ! which sed > /dev/null ; then
+if ! command -v sed > /dev/null ; then
 	echo
 	echo "You must install 'sed' on your build machine"
 	exit 1
 fi
 
 # Check make
-MAKE=$(which make 2> /dev/null)
+MAKE=$(command -v make 2> /dev/null)
 if [ -z "$MAKE" ] ; then
 	echo
 	echo "You must install 'make' on your build machine";
@@ -87,9 +87,9 @@  if [ $MAKE_MAJOR -lt 3 ] || [ $MAKE_MAJOR -eq 3 -a $MAKE_MINOR -lt 81 ] ; then
 fi;
 
 # Check host gcc
-COMPILER=$(which $HOSTCC_NOCCACHE 2> /dev/null)
+COMPILER=$(command -v $HOSTCC_NOCCACHE 2> /dev/null)
 if [ -z "$COMPILER" ] ; then
-	COMPILER=$(which cc 2> /dev/null)
+	COMPILER=$(command -v cc 2> /dev/null)
 fi;
 if [ -z "$COMPILER" ] ; then
 	echo
@@ -113,9 +113,9 @@  if [ $COMPILER_MAJOR -lt 3 -o $COMPILER_MAJOR -eq 2 -a $COMPILER_MINOR -lt 95 ]
 fi;
 
 # check for host CXX
-CXXCOMPILER=$(which $HOSTCXX_NOCCACHE 2> /dev/null)
+CXXCOMPILER=$(command -v $HOSTCXX_NOCCACHE 2> /dev/null)
 if [ -z "$CXXCOMPILER" ] ; then
-	CXXCOMPILER=$(which c++ 2> /dev/null)
+	CXXCOMPILER=$(command -v c++ 2> /dev/null)
 fi
 if [ -z "$CXXCOMPILER" ] ; then
 	echo
@@ -149,7 +149,7 @@  fi;
 # Check that a few mandatory programs are installed
 missing_progs="no"
 for prog in patch perl tar wget cpio python unzip rsync bc ${DL_TOOLS} ; do
-    if ! which $prog > /dev/null ; then
+    if ! command -v $prog > /dev/null ; then
 	echo "You must install '$prog' on your build machine";
 	missing_progs="yes"
 	if test $prog = "svn" ; then
@@ -170,7 +170,7 @@  fi
 
 if grep ^BR2_TOOLCHAIN_BUILDROOT=y $BUILDROOT_CONFIG > /dev/null && \
    grep ^BR2_ENABLE_LOCALE=y       $BUILDROOT_CONFIG > /dev/null ; then
-   if ! which locale > /dev/null ; then
+   if ! command -v locale > /dev/null ; then
        echo
        echo "You need locale support on your build machine to build a toolchain supporting locales"
        exit 1 ;
@@ -184,7 +184,7 @@  fi
 
 if grep -q ^BR2_PACKAGE_CLASSPATH=y $BUILDROOT_CONFIG ; then
     for prog in javac jar; do
-	if ! which $prog > /dev/null ; then
+	if ! command -v $prog > /dev/null ; then
 	    echo >&2
 	    echo "You must install '$prog' on your build machine" >&2
 	    exit 1