Re: [PATCH v2 0/0] AC_CONFIG_MACRO_DIR && non-existent directory

[Stefano Lattarini]

On 02/11/2013 01:11 PM, Pavel Raiskup wrote:
> Hi, thanks for your comments!
>
Thanks for your patches, and you patience.

> Here is second iteration of my proposal.
> 
> [PATCH 1/2] aclocal: just warn if the primary local m4 dir doesn't
>  aclocal.in             | 10 ++++++++++
>  t/aclocal-macrodir.tap | 22 +++++++++++++++++++++-
> 
> [PATCH 2/2] aclocal: fix for more-than-once specified directories
>  aclocal.in             | 31 +++++++++++++++++++++++--------
>  t/aclocal-macrodir.tap | 12 ++++++++----
> 
Review will follow later today, or in the next days.

>> Two meta-questions first, though:
>>
>>   - Do you have copyright assignment paperwork for Automake in place?
>>     Your changes definitely don't qualify as "trivial" or "very short",
>>     so I fear the paperwork is required.
> 
> These shouldn't be needed as Red Hat should have already signed papers
> with FSF covering all employees - if the patch it is part of the work -
> Which is this case.
> 
Nice!  And thanks to Red Hat for that.

>>   - In the future, if it doesn't take you too much time, could you try
>>     to send the patches in-line rather than as attachments? (You can
>>     use the excellent "git send-email" command for that).  This would
>>     make it more convenient for reviewers to answer to your patches
>>     separately, and to quote relevant chunks from them.  Thanks.
> 
> From now I'm using git send-email,
>
Thanks.

> sorry for inconvenience.
> 
And please, no need to apologize!

>>> See:
>>> <http://lists.gnu.org/archive/html/bug-automake/2013-01/msg00115.html>
>>>
>>> * aclocal.in (scan_m4_files): Unique multiply specified directories.
>>>
>> s/Unique multiply/uniquify/ perhaps?
>>
>> Good catch, BTW!  I think we also need a testsuite addition to cover for
>> the use case fixed by this patch (not a requirement to get your patches
>> in; if you don't want to write such a test, I'll get to it myself
>> eventually).
> 
> I tried to write a test.
> Feel free to trash/rewrite it just to speed this process up.
>
OK.

>> =====
>> ===  PATCH [3/3]
>> =====
>>
>>> Subject: [PATCH 3/3] docs: Mention consequences with AC_CONFIG_MACRO_DIRS
>>>
>> I'm quite unconvinced of the usefulness of this patch.  More details below.
>>
>> [...]
>>
>>> diff --git a/doc/automake.texi b/doc/automake.texi
>>> index e700ab9..944fd9d 100644
>>> --- a/doc/automake.texi
>>> +++ b/doc/automake.texi
>>> [...]
>>> +There is strictly encouraged not to change the 'm4' directory name to
>>> +some different name at the moment.
>>>
>> Why not?  Have you examples of real-world issues caused by different
>> names?
> 
> Ok, I was considering situation that we can live without ACLOCAL_AMFLAGS.
> Because we can not, there is no problem (I'm thinking about gettext).
> 
> For this reason, do you think it would be possible to freeze
> ACLOCAL_AMFLAGS and AC_CONFIG_MACRO_DIR a 'little' bit longer in automake
> then until 1.14?
>
Do you mean not removing ACLOCAL_AMFLAGS in 1.14?  Sure, that has never
been the plan.  ACLOCAL_AMFLAGS will cause non-fatal warnings in 1.14,
but will still work.  And very likely it will keep working with Automake
1.15 (and possibly 1.16 too).

<ASIDE>
Notice that if the new versioning and releasing scheme planned at
http://debbugs.gnu.org/13578 is finally picked up, what we are now
labelling as Automake 1.14 will actually be released as Automake 2.0
(to be released no sooner than an year and a half from now); similarly,
1.15 will actually be released as 3.0, and 1.16 as 4.0, and so on.
</ASIDE>

> I'm afraid that all tools will have problems to follow these
> changes...
>
Indeed, we should tread carefully here.

>>> [SNIP]
>>
>>> + The least problematic way seems to be now:
>>> +
>>> address@hidden
>>> +configure.ac: AC_CONFIG_MACRO_DIRS([m4])
>>> +Makefile.am:  # **NO** ACLOCAL_AMFLAGS
>>> address@hidden example
>>> +
>> Alas, this is not true; the above will not work with Automake 1.12.x,
> 
> I forgot about it, sorry.
> 
>> nor with the (so far) latest version of libtool (2.4.2).
> 
> This is because of AC_CONFIG_MACRO_DIRS, there should be
> AC_CONFIG_MACRO_DIR.  But anyway, this is not a good idea.. as we need
> ACLOCAL_AMFLAGS.
> 
>> For the moment, all the users not willing to require cutting-edge
>> autotools for their build systems will still have to specify
>> ACLOCAL_AMFLAGS.
> 
> Yes, but not if they don't want (or don't necessarily need) to switch
> macros into different directory than 'm4'?
>
There might be a misunderstanding here (either from your part, or in
my understanding of what you are saying).  Are you aware that none of
the autotools use *any* local m4 directory if it is not explicitly
instructed to, right?  So there is not "default local m4 directory".
The 'AC_CONFIG_MACRO_DIR' and 'AC_CONFIG_MACRO_DIRS' macros are not
used to switch to an alternate name for the local m4 directory, but
to declare such directory (or directories).

> This is somehow complicated
> and inconsistent behavior among autotools :( and I'm still not sure what
> should I suggest to package maintainers which are dependant on autotools
> for that issue.
>
> Should we suggest in general something like this?
>
>   configure.ac: AC_CONFIG_MACRO_DIR([*Whatever*])
>   Makefile.am:  ACLOCAL_AMFLAGS = -I *Whatever*
>   # but *Whatever* must be the same on both lines
>
I'd suggest them to either:

 - use AC_CONFIG_MACRO_DIRS if they can/want require cutting-edge
   versions of the autotools;

 - use ACLOCAL_AMFLAGS otherwise, for the time being (until the
   now-cutting-edge autotools version have become widespread and
   "old" enough).

Also notice that proper support for AC_CONFIG_MACRO_DIRS will only
be present from the *next* versions of autoconf and libtool (2.70
and 2.6 respectively I think); it is not yet available with the
current one (2.69 and 2.4.2, respectively).

>> =====
>> ===  FIXUP PATCH
>> =====
>>
>>> [PATCH] tests: Command 'aclocal -Inon-existent' should warn
>>>
>>> Check that this command just warns and does not fail.
>>>
>>> See:
>>> <http://lists.gnu.org/archive/html/bug-automake/2013-01/msg00115.html>
>>>
>>> * t/aclocal-macrodir.tap: Check for proper return value and do not handle
>>> warnings as errors.
>>>
>>
>> This patch should just be folded into the first one IMHO, to keep spurious
>> test failures to appear (albeit temporarily) in the testsuite.
> 
> Done.
> 
>> Oops. that "(1)" is just an old copy & paster error; so just drop the "(0)"
>> in your new message.
> 
> Done.
> 
>>>  cat > configure.ac << 'END'
>>>  AC_INIT([oops], [1.0])
>>>  AC_CONFIG_MACRO_DIR([non-existent])
>>>  END
>>>
>>> -not $ACLOCAL -Wnone 2>stderr \
>>> +$ACLOCAL -Wno-error 2>stderr \
>>>   && cat stderr >&2 \
>>>   && grep "couldn't open directory 'non-existent'" stderr \
>>>   || r='not ok'
>>>
>>
>> You should also adjust the sister test 't/aclocal-macrodir.tap'
>> in the same way (there are three checks to adjust there).
> 
> If you think something like that
> 
>  test ! -d foo \
> -  && $ACLOCAL --install --system-acdir ./acdir \
> +  && $ACLOCAL --install --system-acdir ./acdir 2>stderr \
> +  && cat stderr >&2 \
> +  && not grep "couldn't open directory" stderr \
>    && diff acdir/bar.m4 foo/bar.m4 \
>    || r='not ok'
> 
> then it should fixed.
>
Yes, this is good enough IMO.  If I want to further tweak it,
I can do that myself, without bothering you further.

Thanks, and best regards,
  Stefano