Re: [PATCH] `compile' with spaces in arguments

[Ralf Wildenhues]

* Alexandre Duret-Lutz wrote on Fri, Sep 10, 2004 at 08:37:50PM CEST:
> >>> "Ralf" == Ralf Wildenhues <address@hidden> writes:
>  Ralf> Currently, the `compile' script does not allow spaces in arguments.
> 
> Thanks.  I'm installing the following variation of this patch on
> HEAD and branch-1-9.
> 
> One thing I learnt about the cycling "$@" idiom is to `shift'
> only after you have re-`set' the arguments.  This way you ensure
> "$@" is never empty and do not need the ${1-"$@"} cruft. 
> (Release 1.8.2 was essentially a fix for this...)

Thanks for the tip!

A couple of comments:

*snip*
> +eat=
> +
> +for arg
> +do
> +  if test -n "$eat"; then

Please do not use test here, since it might fork, and that several times
at this point.  This enhances complaints about `compile' being slow.
`case' is safe.

> +    eat=
> +  else
> +    case $1 in
> +      -o)
> +     # configure might choose to run compile as `compile cc -o foo foo.c'.
> +     # So we strip `-o arg' only if arg is an object.
> +     eat=1
> +     case $2 in
> +       *.o | *.obj)
> +         ofile=$2
> +         ;;
> +       *)
> +         set x "$@" -o "$2"
> +         shift
> +         ;;
> +     esac
> +     ;;
> +      *.c)
> +     cfile=$1
> +     set x "$@" "$1"
> +     shift
> +     ;;
> +      *)
> +     set x "$@" "$1"
> +     shift
> +     ;;
> +    esac
> +  fi
>    shift
>  done
>  
> @@ -95,35 +99,35 @@
>    # normal compilation that the losing compiler can handle.  If no
>    # `.c' file was seen then we are probably linking.  That is also
>    # ok.
> -  exec "$prog" $args
> +  exec "$@"
>  fi
>  
>  # Name of file we expect compiler to create.
> -cofile=`echo $cfile | sed -e 's|^.*/||' -e 's/\.c$/.o/'`
> +cofile=`echo "$cfile" | sed -e 's|^.*/||' -e 's/\.c$/.o/'`
>  
>  # Create the lock directory.
>  # Note: use `[/.-]' here to ensure that we don't use the same name
>  # that we are using for the .o file.  Also, base the name on the expected
>  # object file name, since that is what matters with a parallel build.
> -lockdir=`echo $cofile | sed -e 's|[/.-]|_|g'`.d
> +lockdir=`echo "$cofile" | sed -e 's|[/.-]|_|g'`.d
>  while true; do
> -  if mkdir $lockdir > /dev/null 2>&1; then
> +  if mkdir "$lockdir" >/dev/null 2>&1; then
>      break
>    fi
>    sleep 1
>  done
>  # FIXME: race condition here if user kills between mkdir and trap.
> -trap "rmdir $lockdir; exit 1" 1 2 15
> +trap "rmdir '$lockdir'; exit 1" 1 2 15
>  
>  # Run the compile.
> -"$prog" $args
> +"$@"
>  status=$?
>  
>  if test -f "$cofile"; then
>    mv "$cofile" "$ofile"
>  fi
>  
> -rmdir $lockdir
> +rmdir "$lockdir"
>  exit $status
>  
>  # Local Variables:
> Index: tests/Makefile.am
> ===================================================================
> RCS file: /cvs/automake/automake/tests/Makefile.am,v
> retrieving revision 1.565.2.2
> diff -u -r1.565.2.2 Makefile.am
> --- tests/Makefile.am 7 Sep 2004 06:26:21 -0000       1.565.2.2
> +++ tests/Makefile.am 10 Sep 2004 18:21:06 -0000
> @@ -93,6 +93,7 @@
>  comment5.test \
>  comment6.test \
>  comment7.test \
> +compile.test \
>  compile_f90_c_cxx.test \
>  compile_f_c_cxx.test \
>  cond.test \
> Index: tests/compile.test
> ===================================================================
> RCS file: tests/compile.test
> diff -N tests/compile.test
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ tests/compile.test        10 Sep 2004 18:21:06 -0000
> @@ -0,0 +1,43 @@
> +#! /bin/sh
> +# Copyright (C) 2004  Free Software Foundation, Inc.
> +#
> +# This file is part of GNU Automake.
> +#
> +# GNU Automake is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# GNU Automake is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Automake; see the file COPYING.  If not, write to
> +# the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> +# Boston, MA 02111-1307, USA.
> +
> +# Make sure `compile' preserves spaces in its arguments.
> +
> +. ./defs || exit 1
> +
> +set -e
> +
> +cp $testsrcdir/../lib/compile .
> +
> +# -o 'a c' should not be stripped because 'a c' is not an object
> +# (it does not matter whether touch creates ./-- or not)
> +./compile touch a.o -- -o 'a c' a.c

Please consider using also args with two or more consecutive spaces.
Those bugs are really tedious to find (like v='a  b'; echo $v).

> +test -f 'a c'
> +test -f ./-o
> +test -f a.o
> +test -f a.c
> +
> +rm 'a c' ./-o a.o a.c
> +
> +./compile touch a.o -- -o 'a c.o' a.c
> +test -f 'a c.o'
> +test ! -f ./-o
> +test ! -f a.o
> +test -f a.c

Regards,
Ralf