[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v2 0/0] AC_CONFIG_MACRO_DIR && non-existent directory
From: |
Pavel Raiskup |
Subject: |
[PATCH v2 0/0] AC_CONFIG_MACRO_DIR && non-existent directory |
Date: |
Mon, 11 Feb 2013 13:11:50 +0100 |
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