| Message ID | 20100120113741.GA31679@pig.zood.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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?
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.
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
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 --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