Hi Ralf,
Den 2010-09-16 20:03 skrev Ralf Wildenhues:
> Hi Peter,
>
> I've looked over the non-testsuite part of this now. Comments below.
> I'll try to get the rest done this weekend.
Ok, great, and thanks!
> * Peter Rosin wrote on Thu, Sep 16, 2010 at 10:50:05AM CEST:
>> --- a/automake.in
>> +++ b/automake.in
>> @@ -396,6 +396,9 @@ my $package_version_location;
>> # TRUE if we've seen AM_ENABLE_MULTILIB.
>> my $seen_multilib = 0;
>>
>> +# TRUE if we've seen AM_PROG_AR
>> +my $seen_ar = 0;
>> +
>> # TRUE if we've seen AM_PROG_CC_C_O
>> my $seen_cc_c_o = 0;
>>
>> @@ -2718,7 +2721,10 @@ sub handle_libraries
>> $var->requires_variables ('library used', 'RANLIB');
>> }
>>
>> - &define_variable ('AR', 'ar', INTERNAL);
>> + if (! $seen_ar)
>> + {
>> + &define_variable ('AR', 'ar', INTERNAL);
>> + }
>
> I don't think the if is necessary. define_variable is a no-op if the
> variable is already defined.
Right. I've had no problems with AC_SUBSTing AR in Libtool, so there's
no reason it should be a problem to add that AC_SUBST in AM_PROG_AR
instead. I haven't detected any problems anyway. I've removed the if
from in my local tree.
>> &define_variable ('ARFLAGS', 'cru', INTERNAL);
>> &define_verbose_tagvar ('AR');
>>
>> @@ -2800,6 +2806,13 @@ sub handle_libraries
>> &check_libobjs_sources ($xlib, $xlib . '_LIBADD');
>> }
>> }
>> +
>> + if (! $seen_ar)
>> + {
>> + msg ('portability', $where,
>> + "`$onelib': linking libraries requires "
>> + . "`AM_PROG_AR' in `$configure_ac'")
>
> I'm kinda wondering whether and how we can make this a bit less daunting
> for the user. Not sure whether to invent another warning category, or
> mention "w32" here, or something else that makes it clear that the world
> isn't going to end. Below too, of course.
But w32 is wrong too, since it doesn't affect GNU on w32. But I see your
point, you want it to be just a hint, not a warning. Maybe it should just
be a diagnostic message which doesn't cause automake -Wall -Werror to fail
at all? I'll wait for more input before I'm doing anything though, but if
a new warning category or any other new infrastructure is needed I don't
think I'm the top candidate for the job.
I'm also not 100% satisfied with the resulting output of the above message,
but that's not very important. Maybe the $onelib thing should be removed?
>> --- a/doc/automake.texi
>> +++ b/doc/automake.texi
>> @@ -3861,6 +3861,15 @@ environment, or use the @option{--with-lispdir}
>> option to
>> @command{configure} to explicitly set the correct path (if you're sure
>> you have an @command{emacs} that supports Emacs Lisp).
>>
>> +...@item AM_PROG_AR([ACT_IF_FAIL])
>
> @ovar{act-if-fail}
Fixed in my local tree. Should I also change the variable name in the
m4 code? (to ACT-IF-FAIL or act-if-fail in that case?)
I'll send an updated version (if needed) after the other half of the review.
Cheers,
Peter
