Re: MSVC: Support for response files with $NM.

[Peter Rosin]

Hi Ralf,

Den 2010-07-06 21:10 skrev Ralf Wildenhues:
> Hi Peter,
>
> * Peter Rosin wrote on Tue, Jul 06, 2010 at 11:27:31AM CEST:
> > Den 2010-07-06 08:07 skrev Ralf Wildenhues:
> > > Did you confirm that the debian/gcc case actually uses the @FILE code
> > > path in the testsuite (should be in the low_cmd_len tests)?
> >
> > No I didn't, but now I have and nm @FILE is used by at least export.at,
> > deplib-in-subdir.at and stresstest.at. And it works just fine for
> > me.
>
> Thanks for verifying.

No problem, but I was initially very confused (and still am) by the
fact that

make check-local TESTSUITEFLAGS='-v -d -k max_cmd_len' \
        INNER_TESTSUITEFLAGS=',export -v -d export' | grep creating

did not catch output from this line:
        func_verbose "creating $NM input file list: $output"

(and no trace of it in either of
tests/testsuite.log,
tests/testsuite.dir/111/testsuite.log or
tests/testsuite.dir/111/testsuite.dir/044/testsuite.log
either)

I had to change func_verbose to echo in the above line to finally
be able to verify.

What's up with that?

> > > > --- a/libltdl/config/ltmain.m4sh
> > > > +++ b/libltdl/config/ltmain.m4sh
> > > > @@ -6719,14 +6719,30 @@ EOF
> > > >             $opt_dry_run || $RM $export_symbols
> > > >             cmds=$export_symbols_cmds
> > > >             save_ifs="$IFS"; IFS='~'
> > > > -           for cmd in $cmds; do
> > > > +           for cmd1 in $cmds; do
> > > >               IFS="$save_ifs"
> > > > -             eval cmd=\"$cmd\"
> > > > +             eval cmd=\"$cmd1\"
> > > >               func_len " $cmd"
> > > >               len=$func_len_result
> > > >               if test "$len" -lt "$max_cmd_len" || test "$max_cmd_len" -le -1; 
> > > > then
> > > >                 func_show_eval "$cmd" 'exit $?'
> > > >                 skipped_export=false
> > > > +             elif test -n "$nm_file_list_spec"; then
> > >
> > > Actually, why even do the expensive test for long command lines, and not
> > > just go for the spec file all the time?  I'm not sure we really want to
> > > do this, as it hampers debugging a bit (the developer doesn't readily
> > > see the contents of the response file), but it surely would be a bit
> > > more efficient.
> >
> > I wanted to change as little as possible. The expensive test was not
> > added by the patch, so I don't feel too bad about that. I didn't
> > even realize it was expensive, but if that's the case it is
> > certainly possible to first test for nm_file_list_spec and only do
> > the max_cmd_len test otherwise.
>
> As I said, I'm not sure which behavior is more desirable; there is a
> tradeoff performance vs. readability of build output to make here.
>
> This was really meant as a point for discussion rather than a patch nit,
> sorry for not being clear enough here.

Ok.

> > > > +               save_libobjs=$libobjs
> > > > +               save_output=$output
> > > > +               output=${output_objdir}/${output_la}.nm
> > > > +               libobjs=$nm_file_list_spec$output
> > > > +               func_append delfiles " $output"
> > > > +               func_verbose "creating $NM input file list: $output"
> > > > +               for obj in $save_libobjs; do
> > > > +                 $ECHO "$obj"
> > > > +               done>   "$output"
> > > > +               eval cmd=\"$cmd1\"
> > >
> > > This eval is wrong and shouldn't be necessary, func_show_eval below
> > > already does an evaluation.  Please check that it is not needed.
> >
> > The double eval was there before, so the patch just copied the
> > existing style. Or didn't it? The code was:
>
> Whatever.

Oh, I didn't mean to imply that I don't care, but I find that I
need 100% attention on getting libtool to do what I want. I just
don't usually have anything left for the bigger picture...

(but perhaps I should also point out that I think you wrote that
part of the patch, and I didn't really look at those aspects of
that code, the code looked fine to me and it did what I wanted)

>            It is wrong to do the double eval for anything here, it's
> only that the func_len test needs one because func_show_eval already
> does an eval and func_len doesn't (and shouldn't).  I'll fix it.

Great! (and it is a feature to have those two changes as separate
commits IMHO)

Cheers,
Peter