Re: [PATCH 1/2] aclocal: just warn if the primary local m4 dir doesn't exist (no error)

[Stefano Lattarini]

Hi Pavel.

There are still some issues with your patches (most of them reported
below).  No need for your to re-roll; I will fix them locally, and then
send the amended patches out for further review before pushing.  Not
sure whether I can do that today though, so be patient.

On 02/11/2013 01:11 PM, Pavel Raiskup wrote:
> Related to automake bug#13514.
> 
> Every bootstrapping process which does not need to have the local
> macro 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
> 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.
> 
> For that reason, we degrade the error to a simple warning.  The
> warning is quite useful for running aclocal by hand - so we are not
> removing completely.
> 
> 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>
> 
> * aclocal.in (SCAN_M4_DIRS_SILENT, SCAN_M4_DIRS_WARN)
> (SCAN_M4_DIRS_ERROR): New constants.
> (scan_m4_dirs): Change the second parameter name to $ERR_LEVEL to
> better reflect new semantic. Use new constants.
> (scan_m4_files): Adjust to reflect the new 'scan_m4_dirs' semantics.
> * t/aclocal-macrodir.tap: Check for proper return value of 'aclocal'
> when AC_CONFIG_MACRO_DIR is specified.  Check whether warning is (is
> not) printed to stderr when the primary macro directory does not
> exist.
> 
> Suggested-by: Ben Pfaff <address@hidden>
> ---
>  aclocal.in             | 31 +++++++++++++++++++++++--------
>  t/aclocal-macrodir.tap | 12 ++++++++----
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/aclocal.in b/aclocal.in
> index b51c09d..8bae156 100644
> --- a/aclocal.in
> +++ b/aclocal.in
> @@ -165,6 +165,11 @@ my @ac_config_macro_dirs;
>  # If set, names a temporary file that must be erased on abnormal exit.
>  my $erase_me;
>  
> +# constants for scan_m4_dirs($ERR_LEVEL) parameter
> +use constant SCAN_M4_DIRS_SILENT => 0;
> +use constant SCAN_M4_DIRS_WARN => 1;
> +use constant SCAN_M4_DIRS_ERROR => 2;
> +
>  ################################################################
>  
>  # Prototypes for all subroutines.
> @@ -355,21 +360,29 @@ sub list_compare (address@hidden@)
>  
>  ################################################################
>  
> -# scan_m4_dirs($TYPE, $ERR_ON_NONEXISTING, @DIRS)
> +# scan_m4_dirs($TYPE, $ERR_LEVEL, @DIRS)
>  # -----------------------------------------------
>  # Scan all M4 files installed in @DIRS for new macro definitions.
>  # Register each file as of type $TYPE (one of the FT_* constants).
> +# Fail without discussion on non-existing include directory when the
> +# $ERR_LEVEL parameter equals to SCAN_M4_DIRS_ERROR, just print warning
> +# when it equals to SCAN_M4_DIRS_WARN and don't complain at all when
> +# it is set to SCAN_M4_DIRS_SILENT.
>  sub scan_m4_dirs ($$@)
>  {
> -  my ($type, $err_on_nonexisting, @dirlist) = @_;
> +  my ($type, $err_level, @dirlist) = @_;
>  
>    foreach my $m4dir (@dirlist)
>      {
>        if (! opendir (DIR, $m4dir))
>       {
>         # TODO: maybe avoid complaining only if errno == ENONENT?
> -       next unless $err_on_nonexisting;
> -       fatal "couldn't open directory '$m4dir': $!";
> +       my $message = "couldn't open directory '$m4dir': $!";
> +
> +       fatal $message if $err_level == SCAN_M4_DIRS_ERROR;
> +       msg ('unsupported', $message) if $err_level == SCAN_M4_DIRS_WARN;
> +       # don't complain if $err_level == SCAN_M4_DIRS_SILENT
> +       next
>       }
>  
>        # We reverse the directory contents so that foo2.m4 gets
> @@ -408,11 +421,13 @@ 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]);
> +      my $first_dir_lvl = $install ? SCAN_M4_DIRS_SILENT : SCAN_M4_DIRS_WARN;
> +      scan_m4_dirs (FT_USER, $first_dir_lvl, $user_includes[0]);
> +      scan_m4_dirs (FT_USER, SCAN_M4_DIRS_ERROR,
> +                 @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, SCAN_M4_DIRS_ERROR, @automake_includes);
> +  scan_m4_dirs (FT_SYSTEM, SCAN_M4_DIRS_ERROR, @system_includes);
>  
>    # Construct a new function that does the searching.  We use a
>    # function (instead of just evaluating $search in the loop) so that
> diff --git a/t/aclocal-macrodir.tap b/t/aclocal-macrodir.tap
> index 3c66e53..4673d71 100755
> --- a/t/aclocal-macrodir.tap
> +++ b/t/aclocal-macrodir.tap
> @@ -105,7 +105,9 @@ mkdir sys-dir the-dir
>  echo 'AC_DEFUN([THE_MACRO], [:])' > sys-dir/my.m4
>  
>  test ! -r the-dir/my.m4 \
> -  && $ACLOCAL --install --system-acdir ./sys-dir \
> +  && $ACLOCAL --install --system-acdir ./sys-dir 2>stderr \
> +  && cat stderr >&2 \
> +  && not grep "couldn't open directory" stderr \
>
This check is not actually necessary; since aclocal is being run with
the "-Wall -Werror" options, it will exit with error if any warning is
triggered.  So we can drop this "grep".

Ditto for similar checks below.

>    && diff sys-dir/my.m4 the-dir/my.m4 \
>    || r='not ok'
>  
> @@ -149,7 +151,9 @@ mkdir acdir
>  echo 'AC_DEFUN([MY_MACRO], [:])' > acdir/bar.m4
>  
>  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'
>  
> @@ -157,14 +161,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"
>  
>  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'
>
We should also check that we can disable the warning with the
'-Wno-unsupported' option.  Will do in my re-roll.

In addition, the sister test case 't/aclocal-macrodirs.tap'
should be adjusted similarly to what has been done for
't/aclocal-macrodir.tap'.  I'll do that in my re-roll.

Thanks,
  Stefano