Re: FYI: ltmain.m4sh needs to handle spaces in paths

[Ralf Wildenhues]

Hi Derek,

* Derek Price wrote on Thu, Jul 07, 2005 at 07:11:40PM CEST:
> Ralf Wildenhues wrote:
> 
> >Oh, I wasn't going to yank it.  That was supposed to be a joke.
> >If you take it as an incentive to fix more cases, the better.  :)
> 
> Okay, the attached patch to 1.5 fixes loads of quoting issues on Windows
> and passes tests on GNU/Linux.  Basically, I made the minimal set of
> changes I needed to to get a large project containing libraries and
> executables to compile on Windows, except that if I set up the elements
> of a variable to be quoted in one location, I quoted them everywhere for
> consistency.

Wow.  Thanks for working on this!

> I then backported the changes to get the tests to pass again on Linux. 
> The backporting to UNIX entailed a few minor changes to libtool.m4,
> demo/Makefile.am, and a few tests.  The test changes basically just
> expect the quotes to appear in output now.
> 
> What I *didn't* do is move my libtool workspace to a name with a space
> and try to run the test suite on LINUX, so there are probably still a
> few changes that need to be made to handle spaces in directories on
> LINUX.  I also didn't try targets with spaces or relative paths within a
> project with spaces.  This should pretty much only handle spaces in `pwd`.

OK.  I would like to test this as much as possible, in order to prevent
regressions, so test cases would be very much necessary.

One big problem of this patch is that it breaks two ways of backward
compatibility.  First, a library installed with a libtool with this
patch cannot be linked against by an older libtool.  Second, the patched
libtool also does not recognize older, already-installed .la files.  For
a stable series like branch-1-5, I really would not like to do this.

OTOH, I believe it is an important goal for libtool to have this added
functionality.  So the way I'd believe would be best would be to put in
branch-1-5 the functionality to _parse_ installed .la files that contain
the quotes (but do not contain spaces in paths), but only enable in HEAD
or maybe branch-2-0 the code that puts in the quotes.  In any case HEAD
should be able to link against older .la files which do not contain the
quoting.

That way, at least there would be a clean upgrade path
  1.5.18 -> 1.5.next -> 2.x
without a trap in 2.x.

Also, we should probably ship an upgrade script which could rewrite
installed .la files to the new notation.

Furthermore, libltdl would need to be adapted as well (and tests written
for that).

Another solution^Whack would be to do the same thing as done previously
with backwards-incompatible extensions: put the new style into a new
variable (e.g., $quoted_dependency_libs).  While somewhat more
convenient, it is also ugly and leads to bloat in the .la files.  Dunno
what's the best here.  :-/

A few small glitches with your patch are noted below for convenience.

Regards,
Ralf

> 2005-07-07  Derek R. Price  <address@hidden>
> 
>     * libtool.m4:  Quote paths in $hardcode_libdir_flag_spec.
>     * ltmain.in:  Quote paths potentially containing spaces.  Use set and
>     "$@" to loop on variables potentially containing quoted elements.
>     * demo/Makefile.am (hc-libflag): Use eval to process quoted
>     arguments inside variables.
>     * tests/link-order.test, tests/link.test: Expect quoted arguments.

> Index: libtool.m4
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/Attic/libtool.m4,v
> retrieving revision 1.314.2.95
> diff -u -p -r1.314.2.95 libtool.m4
> --- libtool.m4        3 Jul 2005 18:15:58 -0000       1.314.2.95
> +++ libtool.m4        7 Jul 2005 17:00:45 -0000
> @@ -2753,7 +2753,7 @@ if test "$GXX" = yes; then
>      _LT_AC_TAGVAR(archive_cmds, $1)='$CC -shared -nostdlib $predep_objects 
> $libobjs $deplibs $postdep_objects $compiler_flags ${wl}-soname $wl$soname -o 
> $lib'
>      _LT_AC_TAGVAR(archive_expsym_cmds, $1)='$CC -shared -nostdlib 
> $predep_objects $libobjs $deplibs $postdep_objects $compiler_flags 
> ${wl}-soname $wl$soname ${wl}-retain-symbols-file $wl$export_symbols -o $lib'
>  
> -    _LT_AC_TAGVAR(hardcode_libdir_flag_spec, $1)='${wl}--rpath ${wl}$libdir'
> +    _LT_AC_TAGVAR(hardcode_libdir_flag_spec, $1)='${wl}--rpath 
> \"${wl}$libdir\"'
>      _LT_AC_TAGVAR(export_dynamic_flag_spec, $1)='${wl}--export-dynamic'
>  
>      # If archive_cmds runs LD, not CC, wlarc should be empty

While ok for most cases, in general it is not ok to quote ${wl}.  For
example, on Solaris it might get expanded to `-Qoption ld '; quoting
like this:  ${wl}\"$libdir\$
should be ok, though.


*big snip*
> Index: ltmain.in
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/Attic/ltmain.in,v
> retrieving revision 1.334.2.76
> diff -u -p -r1.334.2.76 ltmain.in
> --- ltmain.in 3 Jul 2005 16:57:34 -0000       1.334.2.76
> +++ ltmain.in 7 Jul 2005 17:00:49 -0000
> @@ -251,12 +251,12 @@ func_extract_an_archive ()
>      f_ex_an_ar_dir="$1"; shift
>      f_ex_an_ar_oldlib="$1"
>  
> -    $show "(cd $f_ex_an_ar_dir && $AR x $f_ex_an_ar_oldlib)"
> -    $run eval "(cd \$f_ex_an_ar_dir && $AR x \$f_ex_an_ar_oldlib)" || exit $?
> +    $show "(cd \"$f_ex_an_ar_dir\" && $AR x \"$f_ex_an_ar_oldlib\")"
> +    $run eval "(cd \"\$f_ex_an_ar_dir\" && $AR x \"\$f_ex_an_ar_oldlib\")" 
> || exit $?
>      if ($AR t "$f_ex_an_ar_oldlib" | sort | sort -uc >/dev/null 2>&1); then
>       :
>      else
> -      $echo "$modename: ERROR: object name conflicts: 
> $f_ex_an_ar_dir/$f_ex_an_ar_oldlib" 1>&2
> +      $echo "$modename: ERROR: object name conflicts: 
> \"$f_ex_an_ar_dir/$f_ex_an_ar_oldlib\"" 1>&2
>        exit $EXIT_FAILURE
>      fi
>  }
> @@ -1043,7 +1043,7 @@ EOF
>      compiler_flags=
>      linker_flags=
>      dllsearchpath=
> -    lib_search_path=`pwd`
> +    lib_search_path=\"`pwd`\"

This change is not necessary.  The general rule is, that command
substitutions and variable expansions do not get word-split in
assignments and in the word after `case'.

>      inst_prefix_dir=
>  
>      avoid_version=no
> @@ -1458,10 +1458,10 @@ EOF
>         ;;
>       esac
>       case "$deplibs " in
> -     *" -L$dir "*) ;;
> +     *" \"-L$dir\" "*) ;;
>       *)

Oops.  This would need to be
+       *" \"-L$dir\" "* | *" -L$dir "*) ;;

for backward compatibility.  Similarly below.

> -       deplibs="$deplibs -L$dir"
> -       lib_search_path="$lib_search_path $dir"
> +       deplibs="$deplibs \"-L$dir\""
> +       lib_search_path="$lib_search_path \"$dir\""
>         ;;
>       esac
>       case $host in
> @@ -1492,7 +1492,7 @@ EOF
>           ;;
>         *-*-rhapsody* | *-*-darwin1.[012])
>           # Rhapsody C and math libraries are in the System framework
> -         deplibs="$deplibs -framework System"
> +         deplibs="$deplibs \"-framework\" \"System\""
>           continue
>         esac
>       elif test "X$arg" = "X-lc_r"; then

Why do you quote here?

> @@ -1503,7 +1503,7 @@ EOF
>          ;;
>        esac
>       fi
> -     deplibs="$deplibs $arg"
> +     deplibs="$deplibs \"$arg\""
>       continue
>       ;;
>  
> @@ -1814,7 +1814,7 @@ EOF
>  
>        *.$libext)
>       # An archive.
> -     deplibs="$deplibs $arg"
> +     deplibs="$deplibs \"$arg\""
>       old_deplibs="$old_deplibs $arg"
>       continue
>       ;;
> @@ -1831,7 +1831,7 @@ EOF
>         dlprefiles="$dlprefiles $arg"
>         prev=
>       else
> -       deplibs="$deplibs $arg"
> +       deplibs="$deplibs \"$arg\""
>       fi
>       continue
>       ;;
> @@ -1875,7 +1875,17 @@ EOF
>  
>      if test -n "$shlibpath_var"; then
>        # get the directories listed in $shlibpath_var
> -      eval shlib_search_path=\`\$echo \"X\${$shlibpath_var}\" \| \$Xsed -e 
> \'s/:/ /g\'\`
> +      shlib_search_path=
> +      for var in $shlibpath_var; do
> +     eval temp=\$$var
> +     save_IFS=$IFS
> +     IFS=:
> +     for dir in $temp; do
> +       IFS=$save_IFS
> +       shlib_search_path="\"$dir\" $shlib_search_path"
> +     done
> +     IFS=$save_IFS
> +      done
>      else
>        shlib_search_path=
>      fi
> @@ -1925,13 +1935,14 @@ EOF
>      libs=
>      # Find all interdependent deplibs by searching for libraries
>      # that are linked more than once (e.g. -la -lb -la)
> -    for deplib in $deplibs; do
> +    eval set dummy $deplibs; shift
> +    for deplib in ${1+"$@"}; do

No need for ${1+"$@"} here and in all cases below: write
    eval set dummy $deplibs; shift
    for deplib
    do
    ..
instead; it's important the `do' appears on the next line.

>        if test "X$duplicate_deps" = "Xyes" ; then
>       case "$libs " in
>       *" $deplib "*) specialdeplibs="$specialdeplibs $deplib" ;;
>       esac
>        fi
> -      libs="$libs $deplib"
> +      libs="$libs \"$deplib\""
>      done
>  
>      if test "$linkmode" = lib; then
*big snip*

> Index: demo/Makefile.am
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/demo/Attic/Makefile.am,v
> retrieving revision 1.29.2.2
> diff -u -p -r1.29.2.2 Makefile.am
> --- demo/Makefile.am  19 Sep 2004 12:04:52 -0000      1.29.2.2
> +++ demo/Makefile.am  7 Jul 2005 17:00:50 -0000
> @@ -98,7 +98,7 @@ hc-libflag: $(hell_OBJECTS) $(hell_DEPEN
>           echo unsupported > $@ || status="$$?"; \
>         else \
>           echo "$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(hell_OBJECTS) $$flag 
> -L$(libdir) -lhello $(LIBS) $(LIBM)"; \
> -         $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(hell_OBJECTS) $$flag -L$(libdir) 
> -lhello $(LIBS) $(LIBM) || status="$$?"; \
> +         eval "$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(hell_OBJECTS) $$flag 
> -L$(libdir) -lhello $(LIBS) $(LIBM)" || status="$$?"; \
>         fi; \
>         exit $$status
>  
> Index: tests/link-order.test
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/tests/Attic/link-order.test,v
> retrieving revision 1.1.2.1
> diff -u -p -r1.1.2.1 link-order.test
> --- tests/link-order.test     8 Apr 2005 15:17:28 -0000       1.1.2.1
> +++ tests/link-order.test     7 Jul 2005 17:00:50 -0000
> @@ -52,6 +52,7 @@ EOF
>    $libtool --mode=link $CC $CFLAGS -o $srcdir/liba.la $srcdir/a.lo \
>        $srcdir/libb.la -L$prefix_old/lib -lcee -rpath $prefix/lib
>    $libtool --mode=install cp $srcdir/libb.la $prefix/lib/libb.la
> +  #sh -x $libtool --mode=install cp $srcdir/liba.la $prefix/lib/liba.la

Leftover from some test?

>    $libtool --mode=install cp $srcdir/liba.la $prefix/lib/liba.la \
>        >$srcdir/stdout 2>$srcdir/stderr || retcode=1
>    cat $srcdir/stdout
> @@ -60,7 +61,7 @@ done
>  
>  # Do not error if we do not relink (e.g. static-only systems)
>  if $EGREP relinking $srcdir/stderr; then
> -  if $EGREP ' -L.*\/new\/lib -lb -L.*\/old\/lib -lcee' $srcdir/stdout; then 
> :; else
> +  if $EGREP ' "-L.*\/new\/lib" "-lb" "-L.*\/old\/lib" "-lcee"' 
> $srcdir/stdout; then :; else
>      echo "$0: wrong link order" 1>&2
>      retcode=1
>    fi
> Index: tests/link.test
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/tests/link.test,v
> retrieving revision 1.2
> diff -u -p -r1.2 link.test
> --- tests/link.test   30 Jun 2001 20:31:34 -0000      1.2
> +++ tests/link.test   7 Jul 2005 17:00:50 -0000
> @@ -16,7 +16,7 @@ test $? -eq 0 || exit 1
>  
>  echo "$linkresult"
>  case "$linkresult" in
> -*../lib/libnlsut.a) ;;
> +*\"../lib/libnlsut.a\") ;;
>  *)
>    echo "$progname: ../lib/libnlsut.a was not used as expected in linking"
>    exit 1