bug#13514: unintentional regression with AC_CONFIG_MACRO_DIR in 1.13.1

[Stefano Lattarini]

[+cc automake-patches]

Reference:
<http://lists.gnu.org/archive/html/bug-automake/2013-02/msg00041.html>

Related ticket:
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13514>

On 02/08/2013 01:56 PM, Pavel Raiskup wrote:
> Hi Stefano,
> 
Hi Pavel, and thanks for the patches.  Your work is really appreciated!

>>>> [ ... ]
>>>
>>> Ping.  Are you considering my opinions OK?
>>>
>> Basically yes; and a patch would obviously be welcome, if you want
>> to speed up the fixing of this bug ;-)
> 
> First proposal attached :) (0001-*).
>
Thanks.  I've given a cursory look, and they seem generally good (apart
the documentation patch); my review is below.

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.

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

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

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

>>>      wants to search 'm4' dir which does not
>>>      exist.  Ignoring this completely is imo idea;
>>>
>> s/idea/bad idea/ here, I guess.  Right?  IF yes, I mostly agree.
> 
> Yes - typo, sorry.
> 
>> To summarize: for what concerns automake, I think degrading the fatal
>> error in aclocal to a warning is the best and safest approach for now.
>> Improvements or re-tightening can be done later, if needed.
> 
> Done.
> 
> The purpose of patch 0002-* should be obvious from its description.
>
It seems clear enough to me.  Thanks.

> Patches are generated against 1.13.2 branch.
>
Good choice.

> Pavel 

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

> 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):

>    autoreconf: Entering directory `.'
>    autoreconf: running: aclocal --force
>    aclocal: error: couldn't open directory 'm4': No such file or directory
>    autoreconf: aclocal failed with exit status: 1
>
> The problem is that when the aclocal is run for the first time during
> autoreconf, the directory 'm4' does not exist yet.  It will be created by
> e.g. by 'libtoolize' later on.  During the second run (after libtoolize),
> the 'm4' directory exists and aclocal does not complain anything.
>
That trailing "anything" seems superfluous; I'd just remove it.

> For that reason, we degrade the error to warning only
>
s/warning only/a simple warning/

> (when the --install option is not passed).
>
This sentence is confusing, since it gives the impression that the
fatal error will still be present in this case, while the reality is
that not even a warning will be given.  I'd just remove it.

> The warning is quite useful for running aclocal by
> hand - so not removing completely.
>
s/not removing/we are not removing/ perhaps?

> See:
> <http://lists.gnu.org/archive/html/bug-automake/2013-01/msg00115.html>
> <http://lists.gnu.org/archive/html/automake-patches/2010-02/msg00030.html>
>
Thanks for digging out these references.

> * aclocal.in (scan_m4_dirs): Change the $err_on_nonexisting semantic so
> that 2 means error now, 1 is warning.
>
s/is warning/means warning/.

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 ;-)

> Fail or warn only when expected.
> (scan_m4_files): Switch passed values 1 ~> 2 to reflect ^^^.
>
Just "Adjust to reflect the the new 'scan_m4_dirs semantics'" sounds
clearer IMHO.

> Suggested-by: Ben Pfaff <address@hidden>
> ---
> aclocal.in | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/aclocal.in b/aclocal.in
> index b51c09d..6fa8eeb 100644
> --- a/aclocal.in
> +++ b/aclocal.in
> @@ -359,6 +359,9 @@ sub list_compare (address@hidden@)
>  # -----------------------------------------------
>  # Scan all M4 files installed in @DIRS for new macro definitions.
>  # Register each file as of type $TYPE (one of the FT_* constants).
> +# Warn on non-existing include directory when $ERR_ON_NONEXISTING
> +# equals to 1, fail without discussion if it equals to 2 and don't
> +# complain anyting when it equals to zero.
>
s/anyting/at all/

> sub scan_m4_dirs ($$@)
> {
>   my ($type, $err_on_nonexisting, @dirlist) = @_;
> @@ -368,8 +371,14 @@ sub scan_m4_dirs ($$@)
>       if (! opendir (DIR, $m4dir))
>       {
>         # TODO: maybe avoid complaining only if errno == ENONENT?
> +       my $message = "couldn't open directory '$m4dir': $!";
>         next unless $err_on_nonexisting;
> -       fatal "couldn't open directory '$m4dir': $!";
> +
> +       # fail without discussion for non-"primary" macro directory
> +       fatal $message if $err_on_nonexisting == 2;
> +       # just a warning for the "primary" directory
> +       msg ('unsupported', $message);
> +       next
>       }
>
>       # We reverse the directory contents so that foo2.m4 gets
> @@ -409,10 +418,10 @@ sub scan_m4_files ()
>       # Don't complain if the first user directory doesn't exist, in case
>       # we need to create it later (can happen if '--install' was given).
>      scan_m4_dirs (FT_USER, !$install, $user_includes[0]);
> -    scan_m4_dirs (FT_USER, 1, @user_includes[1..$#user_includes]);
> +    scan_m4_dirs (FT_USER, 2, @user_includes[1..$#user_includes]);
>    }
> -  scan_m4_dirs (FT_AUTOMAKE, 1, @automake_includes);
> -  scan_m4_dirs (FT_SYSTEM,   1, @system_includes);
> +  scan_m4_dirs (FT_AUTOMAKE, 2, @automake_includes);
> +  scan_m4_dirs (FT_SYSTEM,   2, @system_includes);
>

=====
===  PATCH [2/3]
=====

> [PATCH 2/3] maint:  Fix for more-than-once specified directories
>
> Do not explore these directories multiple times in 'aclocal'.  This causes
> problems on older packages having specified:
>
s/having specified/that specify/

>    configure.ac:  AC_CONFIG_MACRO_DIRS([m4])
>    Makefile.am:   ACLOCAL_AMFLAGS = -I m4
>
> When the m4 directory does not exist.
>
Since you are not beginning a new sentence here, I'd drop the uppercase.

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

> ---
>  aclocal.in | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/aclocal.in b/aclocal.in
> index 6fa8eeb..2a9e83b 100644
> --- a/aclocal.in
> +++ b/aclocal.in
> @@ -415,6 +415,16 @@ sub scan_m4_files ()
>
>    if (@user_includes)
>      {
> +      # Don't explore the same directory multiple times.  This is here not
> +      # only for speedup purposes.  We need this when user has e.g.
>
s/user/the user/

> +      # specified 'ACLOCAL_AMFLAGS = -I m4' and user has also set
>
s/user //

> +      # AC_CONFIG_MACRO_DIR[S]([m4]) in configure.ac.  This forces the 'm4'
>
s/forces/makes/ perhaps?

> +      # directory occur
>
s/occur/to occur/

> +      # twice here and fail on the second call to
> +      # scan_m4_dirs([m4]) when non existent.
>
s/when non existent/when the 'm4' directory doesn't exist/, I'd say.

> +      # TODO: Should'nt there be rather check in scan_m4_dirs for
>
s/Should'nt/Shouldn't/; s/check/a check/

> +      #       @user_includes[0]?
> +      @user_includes = uniq @user_includes;
> +
>       # Don't complain if the first user directory doesn't exist, in case
>       # we need to create it later (can happen if '--install' was given).
>       scan_m4_dirs (FT_USER, !$install, $user_includes[0]);

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

> * doc/automake.texi: Document possible problems if user wants to specify
> AC_CONFIG_MACRO_DIRS to point somewhere else then to the directory 'm4'.
>
> See:
> <http://lists.gnu.org/archive/html/bug-automake/2013-01/msg00115.html>
> ---
>  doc/automake.texi | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/doc/automake.texi b/doc/automake.texi
> index e700ab9..944fd9d 100644
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -3614,6 +3614,23 @@ AC_CONFIG_MACRO_DIRS([m4])
>  @command{aclocal} will then take care of automatically adding @file{m4/}
> to its search path for m4 files.
>
> +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?

> [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,
nor with the (so far) latest version of libtool (2.4.2).  For the
moment, all the users not willing to require cutting-edge autotools
for their build systems will still have to specify ACLOCAL_AMFLAGS.

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

> ---
> t/aclocal-macrodir.tap | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/aclocal-macrodir.tap b/t/aclocal-macrodir.tap
> index 3c66e53..f989640 100755
> --- a/t/aclocal-macrodir.tap
> +++ b/t/aclocal-macrodir.tap
> @@ -157,14 +157,14 @@ test_end
>
>  #---------------------------------------------------------------------------
>
> -test_begin "AC_CONFIG_MACRO_DIR([non-existent]) errors out (1)"
> +test_begin "AC_CONFIG_MACRO_DIR([non-existent]) warns but succeeds (0)"
>
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).

=====

Thanks again for all your work on this!

Best regards,
  Stefano