Re: [PATCH] Add new 'AM_PROG_AR' macro, triggering the 'ar-lib' script.

[Peter Rosin]

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).
>>  
>> address@hidden 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