Re: [patch] 1.5.14: Incorrect archive member renaming with GNU ar

[Ralf Wildenhues]

Hi Peter,

* Peter O'Gorman wrote on Sat, Apr 16, 2005 at 05:30:01PM CEST:
> Peter O'Gorman wrote:
> |>>
> |>> This is Alexandre Oliva's patch with a test case. OK to apply to
> |>> branch-1-5
> |>> and forward port?
> |>
> |> Yes.  Please show the forward-port, though.  Nice and clean fix, BTW.
> 
> This is the forward port to HEAD. Note that the cross compiling test is
> broken, neither $build nor $host are set for the autotest tests.

Then they need to be set
  eval `$LIBTOOL --config | grep host=`
  eval `$LIBTOOL --config | grep build=`
should work.

Hmm.  Maybe we should consider using
  eval `$LIBTOOL --config | grep EGREP=`
  eval `$LIBTOOL --config | $EGREP '(host|build)=`
when lines like these start to accumulate.  (Just thinking out loud.)

> Okay to apply?

Nits below.  Other than them (and the fact that I haven't tested),
please apply.

> from  Alexandre Oliva  <address@hidden>,
>       Peter O'Gorman  <address@hidden>
> 
>       * config/ltmain.m4sh: Don't add files with the same base name to an 
>       archive; rename duplicates instead.
>       (func_extract_an_archive): Remove section to deal with duplicate
>       base names in an archive.
>       * tests/functests.at: remove.
>       * tests/duplicate_members.at: new test.
>       * tests/testsuite.at: Add new test, remove old.
>       * Makefile.am: Add new test, remove old.
> 
> Index: config/ltmain.m4sh
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/config/ltmain.m4sh,v
> retrieving revision 1.61
> diff -u -r1.61 ltmain.m4sh
> --- config/ltmain.m4sh 14 Apr 2005 11:59:10 -0000 1.61
> +++ config/ltmain.m4sh 16 Apr 2005 15:24:45 -0000
> @@ -987,30 +987,7 @@
>      if ($AR t "$f_ex_an_ar_oldlib" | sort | sort -uc >/dev/null 2>&1); then
>       :
>      else
> -      func_basename "$f_ex_an_ar_oldlib"
> -      f_ex_an_ar_lib="$func_basename_result"
> -      func_warning "$modename: warning: object name conflicts; renaming 
> object files" 1>&2
> -      func_warning "$modename: warning: to ensure that they will not 
> overwrite" 1>&2
> -      func_show_eval "cp \$f_ex_an_ar_oldlib 
> \$f_ex_an_ar_dir/\$f_ex_an_ar_lib"
> -      $AR t "$f_ex_an_ar_oldlib" | sort | uniq -c | while read count name
> -      do
> -     # We don't want to do anything to objects with unique names
> -        test "$count" -eq 1 && continue
> -     i=1
> -     while test "$i" -le "$count"
> -       do
> -       # Put our $i before any first dot (extension)
> -       # Never overwrite any file
> -       name_to="$name"
> -       while test "X$name_to" = "X$name" || test -f 
> "$f_ex_an_ar_dir/$name_to"
> -         do
> -         name_to=`$ECHO "X$name_to" | $Xsed -e "s/\([[^.]]*\)/\1-$i/"`
> -       done
> -       func_show_eval "(cd \$f_ex_an_ar_dir && $AR x \$f_ex_an_ar_lib 
> '$name' && $MV '$name' '$name_to' && $AR d \$f_ex_an_ar_lib '$name')" || exit 
> $?
> -       i=`expr $i + 1`
> -     done
> -      done
> -      func_show_eval "$RM \$f_ex_an_ar_dir/\$f_ex_an_ar_lib"
> +      func_fatal_error "object name conflicts in archive: 
> $f_ex_an_ar_dir/$f_ex_an_ar_oldlib"
>      fi
>  }
>  
> @@ -6304,6 +6281,50 @@
>         oldobjs="$oldobjs $func_extract_archives_result"
>       fi
>  
> +     # POSIX demands no paths to be encoded in archives.  We have
> +     # to avoid creating archives with duplicate basenames if we
> +     # might have to extract them afterwards, e.g., when creating a
> +     # static archive out of a convenience library, or when linking
> +     # the entirety of a libtool archive into another (currently
> +     # not supported by libtool).
> +     if (for obj in $oldobjs
> +         do
> +           $ECHO "X$obj" | $Xsed -e 's%^.*/%%'

Please use func_basename here, similar to the deleted chunk below, as
it's faster.

> +         done | sort | sort -uc >/dev/null 2>&1); then
> +       :
> +     else
> +       $ECHO "copying selected object files to avoid basename conflicts..."
> +
> +       
> +       gentop="$output_objdir/${outputname}x"
> +       generated="$generated $gentop"
> +       func_mkdir_p "$gentop"
> +       save_oldobjs=$oldobjs
> +       oldobjs=
> +       counter=1
> +       for obj in $save_oldobjs
> +       do
> +         objbase=`$ECHO "X$obj" | $Xsed -e 's%^.*/%%'`

Same as above.

> +         case " $oldobjs " in
> +         " ") oldobjs=$obj ;;
> +         *[[\ /]]"$objbase "*)
> +           while :; do
> +             # Make sure we don't pick an alternate name that also
> +             # overlaps.
> +             newobj=lt$counter-$objbase
> +             counter=`expr $counter + 1`
> +             case " $oldobjs " in
> +             *[[\ /]]"$newobj "*) ;;
> +             *) if test ! -f "$gentop/$newobj"; then break; fi ;;
> +             esac
> +           done
> +           func_show_eval "ln $obj $gentop/$newobj || cp $obj 
> $gentop/$newobj"
> +           oldobjs="$oldobjs $gentop/$newobj"
> +           ;;
> +         *) oldobjs="$oldobjs $obj" ;;
> +         esac
> +       done
> +     fi
>       eval cmds=\"$old_archive_cmds\"
>  
>       if len=`expr "X$cmds" : ".*"` &&
> @@ -6317,21 +6338,6 @@
>         objlist=
>         concat_cmds=
>         save_oldobjs=$oldobjs
> -       # GNU ar 2.10+ was changed to match POSIX; thus no paths are
> -       # encoded into archives.  This makes 'ar r' malfunction in
> -       # this piecewise linking case whenever conflicting object
> -       # names appear in distinct ar calls; check, warn and compensate.
> -         if (for obj in $save_oldobjs
> -         do
> -           func_basename "$obj"
> -           $ECHO "$func_basename_result"
> -         done | sort | sort -uc >/dev/null 2>&1); then
> -         :
> -       else
> -         func_warning "object name conflicts; overriding AR_FLAGS to 'cq'"
> -         func_warning "to ensure that POSIX-compatible ar will work"
> -         AR_FLAGS=cq
> -       fi
>         # Is there a better way of finding the last object in the list?
>         for obj in $save_oldobjs
>         do
> Index: tests/duplicate_members.at
> ===================================================================
> RCS file: tests/duplicate_members.at
> diff -N tests/duplicate_members.at
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ tests/duplicate_members.at 16 Apr 2005 15:24:45 -0000
> @@ -0,0 +1,80 @@
> +# Hand crafted tests for GNU Libtool.                         -*- Autotest 
> -*-
> +# Copyright 2004 Free Software Foundation, Inc.

2005.  Thanks for correcting my other bogus patch w.r.t this!

> +
> +# This program 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.
> +
> +# This program 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 this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> +# 02111-1307, USA.
> +
> +AT_BANNER([Testing libtool functions])
> +AT_SETUP([duplicate members in archive tests])
> +
> +
> +LIBTOOL=${abs_top_builddir}/libtool
> +
> +# we don't want to use whole_archive_flag_spec, even if available
> +sed -e 's|^whole_archive_flag_spec=.*|whole_archive_flag_spec=|g' < $LIBTOOL 
> > libtool
> +
> +chmod +x ./libtool

Why not set
  LIBTOOL=./libtool
here and use that from now on?

> +AT_DATA(duplicate_members,
> +[[
> +#! /bin/sh

This ought to be
[[#! /bin/sh

because the shebang has to be on the first line.

Then again, why do you create another script?  You could just execute
the commands you want to run in this one -- is there any special reason?

> +# duplicate members
> +
> +for a in 1 2 3 4 5 6
> +do
> +  mkdir $a
> +  echo "int foo$a() {return $a;}" > $a/a.c
> +done
> +eval `./libtool --config | grep ^CC`

I wonder whether we should do this in all the tests.  Our Makefile makes
sure, through TESTS_ENVIRONMENT, that $CC is set.

> +
> +for a in 1 2 3 4 5 6
> +do
> +  ./libtool --mode=compile --tag=CC $CC -c $CFLAGS -o $a/a.lo $a/a.c
> +done
> +./libtool --mode=link --tag=CC $CC $CFLAGS $LDFLAGS -o libc0.la 6/a.lo
> +./libtool --mode=link --tag=CC $CC $CFLAGS $LDFLAGS -o libconv.la 1/a.lo 
> 2/a.lo 3/a.lo 4/a.lo 5/a.lo libc0.la
> +
> +cat <<EOF > bar.c

AT_DATA (when this is executed in the current test)?

> +int bar() {
> +    int result=foo1() +foo2() +foo3() +foo4() +foo5() +foo6();
> +    return result;
> +}
> +EOF
> +
> +./libtool --mode=compile --tag=CC $CC -c $CFLAGS -o bar.lo bar.c
> +./libtool --mode=link --tag=CC $CC $CFLAGS $LDFLAGS -o libbar.la bar.lo 
> libconv.la -rpath /notexist

Funny thing to note while I read this (has nothing to do with your
patch).  A recent Debian-specific bug caused /nonexistent to be created
because it was $HOME for some system user id.  Wonder whether anything
like this could ever backfire..  :)

> +
> +cat <<EOF > main.c

Same as above.

> +int bar();
> +int main()
> +{
> +if (bar() == 21) return 0;
> +return 1;
> +}
> +EOF
> +./libtool --mode=compile --tag=CC $CC -c $CFLAGS -o main.lo main.c
> +./libtool --mode=link --tag=CC $CC $CFLAGS $LDFLAGS -o main main.lo 
> ./libbar.la
> +]])
> +
> +chmod +x ./duplicate_members
> +./duplicate_members
> +
> +if test "X$host" != "X$build"; then 
> +  AT_CHECK([test -x ./main],[0],[ignore],[ignore])

The `test -x' is ok, but even when cross-compiling, I would like that we
try to execute it, but just not fail if it doesn't work.  Reason is that
you might want to cross-compile for, say, older versions of your system
(i686 -> i486).  No strong opinion, though, except it'd be nice if we
did it consistently.

Other than that, you certainly don't want to ignore the output here.

> +else
> +  AT_CHECK([./main],[0],[ignore],[ignore])
> +fi
> +
> +AT_CLEANUP

Regards,
Ralf