diff mbox

Re: Stop using "which" in ./configure

Message ID 20100120113741.GA31679@pig.zood.org
State New
Headers show

Commit Message

Loïc Minier Jan. 20, 2010, 11:37 a.m. UTC
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.

Comments

Paolo Bonzini Jan. 20, 2010, 12:19 p.m. UTC | #1
On 01/20/2010 12:37 PM, Loïc Minier wrote:
> +    # not found
> +    IFS="$local_ifs"

If you do this, you should set IFS to space-tab-lf at the beginning of 
the script, like this:

IFS=" ""	""
"

or this (better because it does not rely on embedding whitespace 
characters within the line):

IFS=`printf ' \t'`"
"

Paolo
Loïc Minier Jan. 20, 2010, 1:49 p.m. UTC | #2
On Wed, Jan 20, 2010, Paolo Bonzini wrote:
> On 01/20/2010 12:37 PM, Loïc Minier wrote:
> >+    # not found
> >+    IFS="$local_ifs"
> If you do this, you should set IFS to space-tab-lf at the beginning of 
> the script, like this:
> 
> IFS=" ""	""
> "

 Are you saying that I can't backup/restore IFS without setting it
 first?  That would be odd; it works with bash, dash, and zsh here.
 Pointers to affected shell would help me avoid other issues with them.

 Alternatively, I could set IFS in a subshell.

 I checked the autoconf Portable Shell Programming section, but didn't
 find a similar recommendation:
   http://www.gnu.org/software/autoconf/manual/autoconf.html#index-IFS-1639

> or this (better because it does not rely on embedding whitespace 
> characters within the line):
> IFS=`printf ' \t'`"
> "

 If we go that route, perhaps IFS="`printf ' \t\n'`" would be more
 readable?  I'm not sure how common printf is though.
Paolo Bonzini Jan. 20, 2010, 2:18 p.m. UTC | #3
On 01/20/2010 02:49 PM, Loïc Minier wrote:
> On Wed, Jan 20, 2010, Paolo Bonzini wrote:
>> On 01/20/2010 12:37 PM, Loïc Minier wrote:
>>> +    # not found
>>> +    IFS="$local_ifs"
>> If you do this, you should set IFS to space-tab-lf at the beginning of
>> the script, like this:
>>
>> IFS=" ""	""
>> "
>
>   Are you saying that I can't backup/restore IFS without setting it
>   first?

Yes, it affects the behavior of read for example:

$ echo a b c | (read a b c; echo $a; echo $b; echo $c)
a
b
c
$ IFS=
$ echo a b c | (read a b c; echo $a; echo $b; echo $c)
a b c


$

(It's not used by QEMU's configure, but it's better to be defensive).

>> or this (better because it does not rely on embedding whitespace
>> characters within the line):
>> IFS=`printf ' \t'`"
>> "
>
>   If we go that route, perhaps IFS="`printf ' \t\n'`" would be more
>   readable?  I'm not sure how common printf is though.

I tried that, but backtick strips trailing newlines (or something like 
that but anyway it does not work).  IFS=`printf ' \n\t'` would work (the 
double quotes are not needed) but the Autoconf manual suggests 
space-tab-newline in that order (even though only the leading space 
seems important from the rest of the paragraph).

printf is fine, though it may not be a builtin.  It's actually more 
portable (even if slower on some shells) to use printf than echo if you 
have variable substitutions in the string, because it handles 
consistently the case when the output starts with a minus sign.

Paolo
Loïc Minier Jan. 20, 2010, 4:51 p.m. UTC | #4
On Wed, Jan 20, 2010, Paolo Bonzini wrote:
> >  Are you saying that I can't backup/restore IFS without setting it
> >  first?
> Yes, it affects the behavior of read for example:
> $ echo a b c | (read a b c; echo $a; echo $b; echo $c)
> a
> b
> c
> $ IFS=
> $ echo a b c | (read a b c; echo $a; echo $b; echo $c)
> a b c
> 
> $
> (It's not used by QEMU's configure, but it's better to be defensive).

 I *do* understand that changing IFS will affect the program, but the
 patch I sent will backup IFS and then restore it; perhaps you missed
 the backup/restore bits:

 +    local_ifs="$IFS"
 [...]
 +    IFS=:
 [...]
 +            IFS="$local_ifs"
 +            return 0
 [...]
 +    IFS="$local_ifs"
 +    return 1

 Do you have an example of how that breaks if IFS isn't ever set in
 ./configure at all?
Måns Rullgård Jan. 20, 2010, 6:11 p.m. UTC | #5
Loïc Minier <lool@dooz.org> writes:

> On Wed, Jan 20, 2010, Paolo Bonzini wrote:
>> >  Are you saying that I can't backup/restore IFS without setting it
>> >  first?
>> Yes, it affects the behavior of read for example:
>> $ echo a b c | (read a b c; echo $a; echo $b; echo $c)
>> a
>> b
>> c
>> $ IFS=
>> $ echo a b c | (read a b c; echo $a; echo $b; echo $c)
>> a b c
>> 
>> $
>> (It's not used by QEMU's configure, but it's better to be defensive).
>
>  I *do* understand that changing IFS will affect the program, but the
>  patch I sent will backup IFS and then restore it; perhaps you missed
>  the backup/restore bits:

It might not be set at all to begin with, in which case you'd set it
the empty string when restoring, and that would change the behaviour.

The POSIX spec says this:

  If IFS is not set, the shell shall behave as if the value of IFS is
  <space>, <tab>, and <newline>

>  +    local_ifs="$IFS"
>  [...]
>  +    IFS=:
>  [...]
>  +            IFS="$local_ifs"
>  +            return 0
>  [...]
>  +    IFS="$local_ifs"
>  +    return 1

If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
Likewise if you set the value first.
Jamie Lokier Jan. 21, 2010, 4:53 p.m. UTC | #6
Måns Rullgård wrote:
>   If IFS is not set, the shell shall behave as if the value of IFS is
>   <space>, <tab>, and <newline>
> 
> If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
> Likewise if you set the value first.

Remove the colon.  The above will wrongly change empty IFS, which
is not the same as unset IFS.

That said, nobody should expect ./configure to work if IFS has an
unusual value anyway.  Probably .configure should just set it to the
standard value at the start.

-- Jamie
Paolo Bonzini Jan. 21, 2010, 8:12 p.m. UTC | #7
On 01/21/2010 05:53 PM, Jamie Lokier wrote:
>> >  If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
>> >  Likewise if you set the value first.
> Remove the colon.  The above will wrongly change empty IFS, which
> is not the same as unset IFS.

local_ifs would never be unset anyway, so it would never trigger.

After all, Loic's code can be considered okay as it was in the first 
place (sorry).  Instead, we should just make sure that no code ever 
unsets IFS.  I committed this recommendation to the Autoconf manual's 
shell portability section, so it's not necessary to add any comment here.

Paolo
diff mbox

Patch

diff --git a/configure b/configure
index baa2800..90b3c18 100755
--- a/configure
+++ b/configure
@@ -27,6 +27,42 @@  compile_prog() {
   $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags > /dev/null 2> /dev/null
 }
 
+# check whether a command is available to this shell (may be either an
+# executable or a builtin)
+has() {
+    type "$1" >/dev/null 2>&1
+}
+
+# search for an executable in PATH
+path_of() {
+    local_command="$1"
+    local_ifs="$IFS"
+    local_dir=""
+
+    # pathname has a dir component?
+    if [ "${local_command#*/}" != "$local_command" ]; then
+        if [ -x "$local_command" ] && [ ! -d "$local_command" ]; then
+            echo "$local_command"
+            return 0
+        fi
+    fi
+    if [ -z "$local_command" ]; then
+        return 1
+    fi
+
+    IFS=:
+    for local_dir in $PATH; do
+        if [ -x "$local_dir/$local_command" ] && [ ! -d "$local_dir/$local_command" ]; then
+            echo "$local_dir/$local_command"
+            IFS="$local_ifs"
+            return 0
+        fi
+    done
+    # not found
+    IFS="$local_ifs"
+    return 1
+}
+
 # default parameters
 cpu=""
 prefix=""
@@ -763,7 +799,7 @@  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"`
+  solinst=`path_of $install`
   if test -z "$solinst" ; 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"
@@ -776,7 +812,7 @@  if test "$solaris" = "yes" ; then
     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"`
+  sol_ar=`path_of ar`
   if test -z "$sol_ar" ; then
     echo "Error: No path includes ar"
     if test -f /usr/ccs/bin/ar ; then
@@ -969,7 +1005,7 @@  fi
 # pkgconfig probe
 
 pkgconfig="${cross_prefix}pkg-config"
-if ! test -x "$(which $pkgconfig 2>/dev/null)"; then
+if ! has $pkgconfig; then
   # likely not cross compiling, or hope for the best
   pkgconfig=pkg-config
 fi
@@ -977,7 +1013,7 @@  fi
 ##########################################
 # Sparse probe
 if test "$sparse" != "no" ; then
-  if test -x "$(which cgcc 2>/dev/null)"; then
+  if has cgcc; then
     sparse=yes
   else
     if test "$sparse" = "yes" ; then
@@ -993,7 +1029,7 @@  fi
 if $pkgconfig sdl --modversion >/dev/null 2>&1; then
   sdlconfig="$pkgconfig sdl"
   _sdlversion=`$sdlconfig --modversion 2>/dev/null | sed 's/[^0-9]//g'`
-elif which sdl-config >/dev/null 2>&1; then
+elif has sdl-config; then
   sdlconfig='sdl-config'
   _sdlversion=`$sdlconfig --version | sed 's/[^0-9]//g'`
 else
@@ -1424,8 +1460,7 @@  EOF
     fi
   else
     if test "$kvm" = "yes" ; then
-      if [ -x "`which awk 2>/dev/null`" ] && \
-         [ -x "`which grep 2>/dev/null`" ]; then
+      if has awk && has 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);}'`
@@ -1694,8 +1729,7 @@  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 has texi2html && has pod2man; then
     docs=yes
   else
     if test "$docs" = "yes" ; then