bug#8168: macros directory not created automatically

[Stefano Lattarini]

On Friday 01 April 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Mon, Mar 14, 2011 at 08:44:03PM CET:
> >   <http://lists.gnu.org/archive/html/automake/2011-03/msg00000.html>
> >   <http://lists.gnu.org/archive/html/bug-automake/2011-03/msg00004.html>
> 
> > Attached is a patch series (2 patches) that should address the issue.
> > The series is based off of maint -- as I'm not yet sure whether it
> > would be better to merge it to master only, or to maint too.
> 
> > Ralf, OK to apply?
> 
> I'm debating a couple of questions:
> 
> Patch 2:
> - Should `--install -I foo/bar/m4' create intermediate directories, or
>   would we suspect a typo?
>
I'd say the latter.  It should be good enough for the all legitimate uses,
and will make the implementation slightly simpler (we can use the builtin
function `mkdir' instead of the `mkpath' function from module File::Path).
Also, in case the users start complaining about this limitation, we can
still easily lift it, without breaking backward-compatibility.

> - Should `--install -I $dir' also create an absolute $dir?
>
I think so.  Why shouldn't it?

> Does it with your patch?
>
Yes, and that is tested in `aclocal-install-mkdir.test'; see this hunk:

  cwd=`pwd`
  case $cwd in
    *$sp*|*$tab*)
      : cannot run this check
      ;;
    *)
      ACLOCAL_TESTSUITE_FLAGS="-I $cwd/bar" $ACLOCAL --install
      ls -l . bar
      test -f bar/my-defs.m4
      ;;
  esac

(Well, in fact there was a typo in this hunk, which I've corrected in
the meantime; see below).

> (I think "no" with both questions, but it would be good to be sure.)
>
The answer to both question is in truth "yes".  Good you asked :-)

> Patch 1:
> - Should the warning/erroring bits differentiate between relative or
>   absolute directory names?  I'm considering to not warn at all about
>   absolute names, as we might not have any control over the tree there.
>
I agree about not having a warning. But a message with `verb' (thus
displayed only when the user requests verbose output) would be useful
also in this case, no?

> - For a relative directory, a warning seems appropriate; and verb is
>   not the right function for that.
>
Well, in truth, I didn't intend for that message to be a warning -- it's
just a verbose mesage that can help in debugging.  I think `verb' is
appropriate for such an usage.  Probably I should fix the ChangeLog
entry to be more consistent with the intended behaviour; i.e., from

    * aclocal.in (scan_m4_dirs): If a user-specified "include
    directory" is unreadable or non-existent, do not issue a
    fatal error anymore, but simply issue a warning, and only
    when running in verbose mode.

to:

    * aclocal.in (scan_m4_dirs): If a user-specified "include
    directory" is unreadable or non-existent, do not issue a
    fatal error anymore, but only give a proper message when
    running in verbose mode.

Would that be better?

>   The most fitting category would be
>   syntax, barring anything better?  (And yes, that means aclocal -Werror
>   would then error out, but that could be considered a good thing).
>   But it seems 'verb' would be appropriate for absolute directories.
> 
> What do you think?
>
I think that we should behave similarly to how gcc, m4 and perl (and
probably many other programs) behave -- i.e., no warning on `-I' used
with non-existent directories, either relative or absolute:
  $ gcc -I /none foo.c
  $ m4 -I /none </dev/null
  $ perl -I /none -e '1;'

> > If yes, where (maint, or master only)?
> 
> Hmm, 1/2 can be seen as a bug fix, but I'd regard 2/2 as a new feature.
> Maybe base both of them off maint, and merge 1/2 to maint when we're
> done with the semantics?
>
Fine with me.

> Further, a couple of trivial nits inline.
> 
> Thanks,
> Ralf

> > Subject: [PATCH 1/2] aclocal: `-I' does not bail out on invalid directories.
> > 
> > Related to automake bug#8168.
> > 
> > * aclocal.in (scan_m4_dirs): If a user-specified "include
> > directory" is unreadable or non-existent, do not issue a
> > fatal error anymore, but simply issue a warning, and only
> > when running in verbose mode.
> > * NEWS: Update.
> > * tests/aclocal-bad-dirlist-include-dir.test: New test.
> > * tests/aclocal-bad-local-include-dir.test: Likewise.
> > * tests/aclocal-bad-system-include-dir.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
> > * tests/.gitignore: Update.

> > --- /dev/null
> > +++ b/tests/aclocal-bad-dirlist-include-dir.test
> 
> Wouldn't "bad-dirlist-include" be a long-enough and precise-enough name?
>
Hmm... I prefero to keep the `aclocal' in there, to keep the test
names esier to search.  So I went for `aclocal-bad-dirlist-incl.test'.
Similarly for the other two tests.

> > +# Check that `aclocal' errors out when passed a non-readable directory
> > +# with the `dirlist' file.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat > configure.in <<END
> > +AC_INIT([$me], [1.0])
> > +END
> > +
> > +mkdir dirlist-test
> > +chmod a-r dirlist-test
> > +ls -l disrlist-test && Exit 77
> 
> Typo disrlist
>
Oops, well spotted.  Fixed.

> > +$ACLOCAL 2>stderr && { cat stderr >&2; Exit 1; }
> 
> Please add a comment before this line:
>   # See the m4/dirlist file in the source tree.
> 
> which I needed to understand why the test was working at all.
>
Done (and I agree the comment is useful).

> > +cat stderr >&2
> > +grep " couldn't open directory .*dirlist-test" stderr


> > --- /dev/null
> > +++ b/tests/aclocal-bad-local-include-dir.test
> 
> > +# Check that `aclocal' does not bail out when passed a non-existent
> > +# or non-readable directory with the `-I' option.  Also check that
> > +# warns appropriately when `--verbose' is used.
> 
> The second sentence is missing a subject ('it'?).
>
Yep.  Fixed.

> > Subject: [PATCH 2/2] aclocal: create local directory where to install m4 
> > files
> 
> > Before this change, a call like `aclocal -I m4 --install' would
> > fail if the `m4' directory wasn't pre-existing.  This could be
> > particularly annoying when running in a checked-out version
> > from a VCS like git, which doesn't allow empty directories to
> > be tracked.
> > 
> > Closes automake bug#8168.
> > 
> > * aclocal.in (install_file): Change signature: take as second
> > argument the destination directory rather than the destination
> > file.  Crate the destination directory if it doesn't already
> > exist.  In verbose mode, tell what is being copied where.
> > (write_aclocal: Update.
> > * NEWS: Update.
> > * THANKS: Update.
> > * tests/aclocal-install-fail.test: New test.
> > * tests/aclocal-install-mkdir.test: Likewise.
> > * tests/aclocal-no-install-no-mkdir.test: Likewise.
> > * tests/aclocal-verbose-install.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
> 
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -10,6 +10,10 @@ New in 1.11.0a:
> >    - aclocal does not issue a fatal error anymore if one of the directories
> >      specified with `-I' does not exist, or is not readable.
> >  
> > +  - If `aclocal --install' is used, and the first directory specified with
> > +    `-I' is non-existent, aclocal will now create it before trying to copy
> > +    files in it.
> > +
> >  Bugs fixed in 1.11.0a:
> >  
> >  * Bugs introduced by 1.11:
> 
> > --- a/aclocal.in
> > +++ b/aclocal.in
> > @@ -207,12 +207,15 @@ sub reset_maps ()
> >    undef &search;
> >  }
> >  
> > -# install_file ($SRC, $DEST)
> > +# install_file ($SRC, $DESTDIR)
> >  sub install_file ($$)
> >  {
> > -  my ($src, $dest) = @_;
> > +  my ($src, $destdir) = @_;
> > +  my $dest = $destdir . "/" . basename $src;
> >    my $diff_dest;
> >  
> > +  verb "installing $src to $dest";
> > +
> >    if ($force_output
> >        || !exists $file_contents{$dest}
> >        || $file_contents{$src} ne $file_contents{$dest})
> > @@ -265,6 +268,8 @@ sub install_file ($$)
> >     }
> >        elsif (!$dry_run)
> >     {
> > +     xsystem ('mkdir', $destdir)
> > +            unless -d $destdir;
> 
> Perl has a mkdir function, there is no need for xsystem.
>
Agreed (and testcases updated accordingly).

> >       xsystem ('cp', $src, $dest);
> >     }
> >      }
> > @@ -778,9 +783,7 @@ sub write_aclocal ($@)
> >           my $dest;
> >           for my $ifile (@{$file_includes{$file}}, $file)
> >             {
> > -             $dest = "$user_includes[0]/" . basename $ifile;
> > -             verb "installing $ifile to $dest";
> > -             install_file ($ifile, $dest);
> > +             install_file ($ifile, $user_includes[0]);
> >             }
> >           $installed = 1;
> >         }
 
> > --- /dev/null
> > +++ b/tests/aclocal-install-fail.test
> 
> > +# Check that `aclocal --install' fails when it should.
> 
> This test should use required=ro-dir I think.
>
Agreed. Fixed.

> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat > configure.in <<END
> > +AC_INIT([$me], [1.0])
> > +MY_MACRO
> > +END
> > +
> > +mkdir dirlist-test
> > +cat > dirlist-test/my-defs.m4 <<END
> > +AC_DEFUN([MY_MACRO], [:])
> > +END
> > +
> > +: > a-regular-file
> > +mkdir unwritable-dir
> > +chmod a-w unwritable-dir
> > +
> > +ACLOCAL_TESTSUITE_FLAGS='-I a-regular-file' $ACLOCAL --install 2>stderr \
> > +  && { cat stderr >&2; Exit 1; }
> > +cat stderr >&2
> > +grep 'mkdir:.*a-regular-file' stderr
> > +
> > +ACLOCAL_TESTSUITE_FLAGS='-I unwritable-dir/sub' $ACLOCAL --install \
> > +  2>stderr && { cat stderr >&2; Exit 1; }
> > +cat stderr >&2
> > +grep 'mkdir:.*unwritable-dir/sub' stderr
> > +
> > +ACLOCAL_TESTSUITE_FLAGS='-I unwritable-dir' $ACLOCAL --install 2>stderr \
> > +  && { cat stderr >&2; Exit 1; }
> > +cat stderr >&2
> > +grep 'cp:.*unwritable-dir' stderr


> > --- /dev/null
> > +++ b/tests/aclocal-install-mkdir.test
> 
> > +# Check that `aclocal --install' create the local m4 directory if
> > +# necessary.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat > configure.in <<END
> > +AC_INIT([$me], [1.0])
> > +MY_MACRO
> > +END
> > +
> > +mkdir dirlist-test
> > +cat > dirlist-test/my-defs.m4 <<END
> > +AC_DEFUN([MY_MACRO], [:])
> > +END
> > +
> > +ACLOCAL_TESTSUITE_FLAGS='-I foo' $ACLOCAL --install
> > +ls -l . foo
> > +test -f foo/my-defs.m4
> > +
> > +cwd=`pwd`
> > +case $pwd in
>
This should be `$cwd'.  Fixed.

> > +  *$sp*|*$tab*)
> > +    : cannot run this check
> > +    ;;
> > +  *)
> > +    ACLOCAL_TESTSUITE_FLAGS="-I $cwd/bar" $ACLOCAL --install
> > +    ls -l . bar
> > +    test -f bar/my-defs.m4
> > +    ;;
> > +esac
> > +
> > +mkdir zardoz
> > +# What should happen:
> > +#  * quux should be created, and required m4 files copied into there.
> > +#  * zardoz shouldn't be preferred to quux, if if the former exists while
> > +#    the latter does not.
> > +#  * blah shouldn't be uselessly created.
> > +ACLOCAL_TESTSUITE_FLAGS='-I quux -I zardoz -I blah' $ACLOCAL --install
> > +ls -l . quux zardoz
> > +grep MY_MACRO quux/my-defs.m4
> > +ls zardoz | grep . && Exit 1
> > +test -d blah || test -r blah && Exit 1


> > --- /dev/null
> > +++ b/tests/aclocal-no-install-no-mkdir.test
> > @@ -0,0 +1,39 @@
> 
> > +# Check that `aclocal' do not create non-existent local m4 directory
> 
> s/do/does/
> a non-existent
>
Fixed.

> > +# if the `--install' option is not given.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat > configure.in <<END
> > +AC_INIT([$me], [1.0])
> > +MY_MACRO
> > +END
> > +
> > +mkdir dirlist-test
> > +cat > dirlist-test/my-defs.m4 <<END
> > +AC_DEFUN([MY_MACRO], [:])
> > +END
> > +
> > +# Ignore the exit status of aclocal; that is checked in other tests.
> 
> Why?
>
Better separation of concerns between different tests (sorta).

> Can't hurt to also test that it fails, no?
>
As you prefer -- I really have no strong feelings here.

> > +ACLOCAL_TESTSUITE_FLAGS='-I foo' $ACLOCAL -I bar || :
> > +test ! -d foo && test ! -r foo
> > +test ! -d bar && test ! -r bar
> 

Thanks,
  Stefano