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

[Pavel Raiskup]

Hi, thanks for your comments!

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 ++++++++----

> 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.

>   - 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, sorry for inconvenience.

>>>> Quickly again (let's say that AC_CONFIG_MACRO_DIR([m4]) is specified):
>>>>   a) Warn because 'aclocal -I m4'
>>>>
>>> It should be just 'aclocal' here.  The point of the new code is that,
>>> if you specify a directory as argument to AC_CONFIG_MACRO_DIR, you
>>> don't need to repeat it again on the aclocal command line nor in
>>> ACLOCAL_AMFLAGS anymore.
>>
>> This may cause problems when user wants to use gettext && specify target
>> directory on a different place than 'm4'.
>>
> Such an issue should be reported to the gettext developers, and at most
> warned about in the gettext manual, rather than in the automake manual.
> We definitely want to support different directory names, if the user have
> reasons to use them.

This is truth, I'll contact gettext mailing list.

> > For these I'm attaching the 0003-* patch.
> >
> I don't truly like it; see below for more details.

The best for now is IMO skip texi edit as I'm not a good person to make
this kind of advices :).

> =====
> ===  PATCH [1/3]
> =====
>
>> Subject: [PATCH 1/3] maint:  Warn only if primary -Idir directory does not 
>> exist
>>
> Style: we use only one space after the colon, and don't upper-case the
> first word after it.  Also, the word before the semicolon should refer
> to the area/tool touched by the patch; so, in this case, the summary line
> should be:
>
>   aclocal: just warn if the primary local m4 dir doesn't exist (no hard error)
>
> Finally, since this series has an associated entry in the bug tracker, it
> would be nice to reference it in your commit message:
>
>   "Related to automake bug#13514"
>
> The same holds for the other patches.

These should be fixed now!

>> Every bootstrapping process which does not need to have the "target" macro
>>
> s/"target"/local/ IMO.
>
>> directory existing in version control system (because it does not have any
>> user-defined macros) would fail during autoreconf -vfi phase if the
>> AC_CONFIG_MACRO_DIRS([m4]) is specified (to force tools to use 'm4' as
>> target directory and to instruct aclocal to look into this directory):
>>
> What about this instead?
>
>   (to force tools like 'autopoint' and 'libtoolize' to use 'm4' as
>   the local directory where to install definitions of their m4 macros,
>   and to instruct aclocal to look into it):

C&P into changelog, thanks.

> s/warning only/a simple warning/
> s/not removing/we are not removing/ perhaps?
> s/is warning/means warning/.
> s/anyting/at all/
> Just "Adjust to reflect the the new 'scan_m4_dirs semantics'" sounds
> clearer IMHO.
> Since you are not beginning a new sentence here, I'd drop the uppercase.
> s/having specified/that specify/
> [... and others ...]

Oh, thanks for this.  I tried to fix all grammar problems you mentioned.

> And honestly, I'd rather use strings than "magic numbers" for the values
> of '$err_on_nonexisting' (and possibly rename the variable); but I can do
> that in a follow-up patch, so no need to change it in yours (unless you
> agree with me ;-)

I agree :) I tried to fix it..

>> 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.

> =====
> ===  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?  I'm afraid that all tools will have problems to follow
these changes...

>> [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'?  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

> =====
> ===  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.

Pavel