Patchwork Re: Stop using "which" in ./configure

login
register
mail settings
Submitter Juan Quintela
Date Jan. 20, 2010, 12:49 p.m.
Message ID <m3pr55aqkt.fsf@neno.neno>
Download mbox | patch
Permalink /patch/43279/
State New
Headers show

Comments

Juan Quintela - Jan. 20, 2010, 12:49 p.m.
Loïc Minier <lool@dooz.org> wrote:
> On Tue, Jan 19, 2010, Måns Rullgård wrote:
> [...]
>> Why the extra variable?  Using $1 directly seems just as obvious to me.
> [...]
>
>  I'm attaching an updated patch which doesn't use this variable.  Should
>  be applied after the sdl-config patch.
>
>> Is the full path of these tools really important?  Doesn't look like
>> it to me.
>
>  I'm attaching a new patch which changes the tests a bit; I would prefer
>  if someone with access to a Solaris build environment would do this
>  though.

Hi

could you test this patch?

I did it concurrently with yours.  my prog_exist() is equal to your
has(), and path_of() is only needed in a single place, that I think it
is better handled with a single grep in that place.

What do you think?

From 4580f69b022e7861bf3be1c694222c0ee82889b7 Mon Sep 17 00:00:00 2001
Message-Id: <4580f69b022e7861bf3be1c694222c0ee82889b7.1263991676.git.quintela@redhat.com>
In-Reply-To: <cover.1263991676.git.quintela@redhat.com>
References: <cover.1263991676.git.quintela@redhat.com>
From: Juan Quintela <quintela@redhat.com>
Date: Wed, 20 Jan 2010 13:24:59 +0100
Subject: [PATCH 1/3] substitute all uses of which by type

Except in one case, we are only interested in knowing if a command
exist, not is path.  Just create prog_exists() function that does this
check.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 configure |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)
Paolo Bonzini - Jan. 20, 2010, 1:16 p.m.
On 01/20/2010 01:49 PM, Juan Quintela wrote:
> +      if prog_exist "awk" -a \
> +         prog_exist "grep"; then

> +  if prog_exist "texi2html" -a \
> +     prog_exist "pod2man" ; then

-a is wrong here.  BTW, it's evil for test too because its precedence 
rules WRT ! and -f/-n/-z/-x/etc. are broken.

Paolo
Loïc Minier - Jan. 20, 2010, 1:54 p.m.
On Wed, Jan 20, 2010, Juan Quintela wrote:
> +prog_exist() {
> +    prog="$1"
> +    type $1 > /dev/null 2> /dev/null
> +}

 You set prog but you use $1; also, it seems vars in functions should be
 prefixed with local_ in qemu's ./configure.  I was told not to use a
 local var here though.

> -  if test "$solinst" = "/usr/sbin/install" ; then
> +  if type install2 2> /dev/null | grep /usr/bin/install >/dev/null ; then

 install2?

> +      if prog_exist "awk" -a \
> +         prog_exist "grep"; then

 already pointed out, but -a is a feature of "test" and incorrect here;
 you want &&

> -  if test -x "`which texi2html 2>/dev/null`" -a \
> -          -x "`which pod2man 2>/dev/null`" ; then
> +  if prog_exist "texi2html" -a \
> +     prog_exist "pod2man" ; then

 ditto


 Perhaps we should avoid work duplication though?

Patch

diff --git a/configure b/configure
index 5631bbb..c1c9bb4 100755
--- a/configure
+++ b/configure
@@ -27,6 +27,11 @@  compile_prog() {
   $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags > /dev/null 2> /dev/null
 }

+prog_exist() {
+    prog="$1"
+    type $1 > /dev/null 2> /dev/null
+}
+
 # default parameters
 cpu=""
 prefix=""
@@ -763,21 +768,19 @@  fi
 # Solaris specific configure tool chain decisions
 #
 if test "$solaris" = "yes" ; then
-  solinst=`which $install 2> /dev/null | /usr/bin/grep -v "no $install in"`
-  if test -z "$solinst" ; then
+  if ! prog_exist "$install" ; then
     echo "Solaris install program not found. Use --install=/usr/ucb/install or"
     echo "install fileutils from www.blastwave.org using pkg-get -i fileutils"
     echo "to get ginstall which is used by default (which lives in /opt/csw/bin)"
     exit 1
   fi
-  if test "$solinst" = "/usr/sbin/install" ; then
+  if type install2 2> /dev/null | grep /usr/bin/install >/dev/null ; then
     echo "Error: Solaris /usr/sbin/install is not an appropriate install program."
     echo "try ginstall from the GNU fileutils available from www.blastwave.org"
     echo "using pkg-get -i fileutils, or use --install=/usr/ucb/install"
     exit 1
   fi
-  sol_ar=`which ar 2> /dev/null | /usr/bin/grep -v "no ar in"`
-  if test -z "$sol_ar" ; then
+  if ! prog_exist "ar" ; then
     echo "Error: No path includes ar"
     if test -f /usr/ccs/bin/ar ; then
       echo "Add /usr/ccs/bin to your path and rerun configure"
@@ -969,7 +972,7 @@  fi
 # pkgconfig probe

 pkgconfig="${cross_prefix}pkg-config"
-if ! test -x "$(which $pkgconfig 2>/dev/null)"; then
+if ! prog_exist "$pkgconfig" ; then
   # likely not cross compiling, or hope for the best
   pkgconfig=pkg-config
 fi
@@ -977,7 +980,7 @@  fi
 ##########################################
 # Sparse probe
 if test "$sparse" != "no" ; then
-  if test -x "$(which cgcc 2>/dev/null)"; then
+  if prog_exist "cgcc" ; then
     sparse=yes
   else
     if test "$sparse" = "yes" ; then
@@ -1419,8 +1422,8 @@  EOF
     fi
   else
     if test "$kvm" = "yes" ; then
-      if [ -x "`which awk 2>/dev/null`" ] && \
-         [ -x "`which grep 2>/dev/null`" ]; then
+      if prog_exist "awk" -a \
+         prog_exist "grep"; then
         kvmerr=`LANG=C $cc $QEMU_CFLAGS -o $TMPE $kvm_cflags $TMPC 2>&1 \
 	| grep "error: " \
 	| awk -F "error: " '{if (NR>1) printf(", "); printf("%s",$2);}'`
@@ -1689,8 +1692,8 @@  fi

 # Check if tools are available to build documentation.
 if test "$docs" != "no" ; then
-  if test -x "`which texi2html 2>/dev/null`" -a \
-          -x "`which pod2man 2>/dev/null`" ; then
+  if prog_exist "texi2html" -a \
+     prog_exist "pod2man" ; then
     docs=yes
   else
     if test "$docs" = "yes" ; then