Re: [PATCH] Allow the use of a listing file if the archiver supports it.

[Peter Rosin]

Den 2010-08-12 19:49 skrev Ralf Wildenhues:
> Hi Peter,
> 
> * Peter Rosin wrote on Thu, Aug 12, 2010 at 03:28:57PM CEST:
>> This is a patch that makes use of @FILE support in the
>> archiver, if the archiver supports it. That makes linking
>> succeed for long command lines when using MSVC, as MSVC
>> can't do piecewise linking (-r -o) which is the current
>> fallback. Absolute paths still needs work, but that will
>> have to wait for Chucks work in that area.
> 
> Absolute paths are not a show stopper, for practical purposes.
> 
> This is ok for master with nits below addressed.

Thanks for the review!

>> Subject: [PATCH] Allow the use of a listing file if the archiver supports it.
>>
>> * libltdl/m4/libtool.m4 (_LT_CMD_OLD_ARCHIVE): Move detection
>> of a suitable archiver...
>> (_LT_PROG_AR): ...to here. New macro, also detect if the
>> archiver supports a listing file with the new variable
>> archiver_list_spec.
>> * libltdl/config/ltmain.m4sh: If the archiver supports a listing
>> file, use it when max_cmd_len is exceeded.  This is needed for
>> tool chains such as MSVC which do not support piece by piece
>> linking (-r -o).
>> * doc/libtool.texi (libtool script contents): Update with
>> archiver_list_spec description.
> 
>> --- a/doc/libtool.texi
>> +++ b/doc/libtool.texi
>> @@ -5875,6 +5875,10 @@ Commands used to create shared libraries, shared 
>> libraries with
>>  @option{-export-symbols} and static libraries, respectively.
>>  @end defvar
>>  
>> address@hidden archiver_list_spec
>> +How to feed a file listing to the archiver.
>> address@hidden defvar
> 
> We already have
> 
> @defvar nm_file_list_spec
> Specify filename containing input files for @code{NM}.
> @end defvar
> 
> and I like that description a wee bit better, with s/NM/AR/.  WDYT?

Agreed.

>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh
>> @@ -7900,6 +7900,16 @@ EOF
>>      len=$func_len_result
>>      if test "$len" -lt "$max_cmd_len" || test "$max_cmd_len" -le -1; then
>>        cmds=$old_archive_cmds
>> +    elif test -n "$archiver_list_spec"; then
>> +      func_verbose "using command file archive linking..."
>> +      for obj in $oldobjs
>> +      do
>> +        $ECHO \""$obj"\"
> 
> Have you tried and ensured that those \"extra\" quotes are necessary
> and no problem?  I'd leave them out otherwise.

They are needed to support spaces in filenames, but the way the loop
is constructed that's just silly anyway. I removed the extra
quotes.

>> +      done > $output_objdir/$libname.libcmd
>> +      save_oldobjs="$oldobjs"
>> +      oldobjs=" $archiver_list_spec$output_objdir/$libname.libcmd"
>> +      eval cmds=\"\$old_archive_cmds\"
> 
> This eval looks spurious, it should be sufficient to just say
>           cmds=$old_archive_cmds
> 
> here.  You do need to remove or move down the following line then,
> however:
> 
>> +      oldobjs="$save_oldobjs"

There are *many* instances of the "eval cmds=" idiom. I fear the patch
that cleans it up though...

It works without the eval, so going that way. The other fallback code
destroys oldobjs anyway so it should be safe to remove both save_oldobjs
lines.

> Rationale I'm being picky here: the extra eval expands '~' in file
> names listed in $old_archive_cmds which then causes failures such as
> <https://buildd.debian.org/fetch.cgi?pkg=libtool&arch=s390&ver=2.2.6b-2~bpo50%2B1&stamp=1266125841&file=log>
> (Yes, I'm still not done with a general fix, but I'd like to avoid
> proliferating the problem where possible.)

/me trembles... :-)

> If my suggestions don't work, then feel free to go ahead with your
> version, I'll make a note to investigate then.

They seem to work fine...

>> --- a/libltdl/m4/libtool.m4
>> +++ b/libltdl/m4/libtool.m4
>> @@ -1312,14 +1312,42 @@ need_locks="$enable_libtool_lock"
>>  ])# _LT_ENABLE_LOCK
>>  
>>  
>> +# _LT_PROG_AR
>> +# -----------
>> +m4_defun([_LT_PROG_AR],
>> +[AC_CHECK_TOOLS(AR, [ar], false)
>> +: ${AR=ar}
>> +: ${AR_FLAGS=cru}
>> +_LT_DECL([], [AR], [1], [The archiver])
>> +_LT_DECL([], [AR_FLAGS], [1], [Flags to create an archive])
>> +
>> +AC_CACHE_CHECK([for archiver @FILE support], [lt_cv_ar_at_file],
>> +  [lt_cv_ar_at_file=no
>> +   AC_COMPILE_IFELSE([[int some_variable = 0;]],
>> +     [echo conftest.$ac_objext > conftest.lst
>> +      am_ar_try='$AR $AR_FLAGS libconftest.a @conftest.lst'
>> +      AC_TRY_EVAL([am_ar_try])
>> +      if test "$ac_status" -eq 0; then
>> +        lt_cv_ar_at_file=@
>> +      fi
>> +      rm -f conftest.* libconftest.a
>> +     ])
>> +  ])
>> +
>> +if test "x$lt_cv_ar_at_file" = xno; then
>> +  archiver_list_spec=
>> +else
>> +  archiver_list_spec=lt_cv_ar_at_file
> 
> There is a $ missing here.  I think this should cause at least some of
> our current tests to fail.

Indeed. *blush*

I tested mingw and msvc, and it worked. Then I changed things trivially
(or so I thought) and retested, but this time forgetting to removing the
`pwd` test from stresstest.at and failed to notice this problem since it
was masked by absolute paths problems...

>> +fi
>> +_LT_DECL([], [archiver_list_spec], [1],
>> +  [How to feed a file listing to the archiver])
>> +])# _LT_PROG_AR
> 

I have pushed as attached.

Cheers,
Peter

0001-Allow-the-use-of-a-listing-file-if-the-archiver-supp.patch.txt
Description: Text document