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: >> + >> +@example >> +configure.ac: AC_CONFIG_MACRO_DIRS([m4]) >> +Makefile.am: # **NO** ACLOCAL_AMFLAGS >> +@end 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
