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: >>> + >>> +@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'? > 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
