[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#13514: [PATCH v2 0/0] AC_CONFIG_MACRO_DIR && non-existent directory

From: Pavel Raiskup
Subject: bug#13514: [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

> For the moment, all the users not willing to require cutting-edge
> autotools for their build systems will still have to specify

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

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


> Oops. that "(1)" is just an old copy & paster error; so just drop the "(0)"
> in your new message.


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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]