Re: bug#8168: macros directory not created automatically

[Ralf Wildenhues]

* 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?
- Should `--install -I $dir' also create an absolute $dir?  Does it with
  your patch?  (I think "no" with both questions, but it would be good
  to be sure.)

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.
- For a relative directory, a warning seems appropriate; and verb is not
  the right function for that.  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?

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

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.

> --- a/NEWS
> +++ b/NEWS
> @@ -5,6 +5,11 @@ New in 1.11.0a:
>    - The `lzma' compression scheme and associated automake option `dist-lzma'
>      is obsoleted by `xz' and `dist-xz' due to upstream changes.
>  
> +* Changes to aclocal:
> +
> +  - aclocal does not issue a fatal error anymore if one of the directories
> +    specified with `-I' does not exist, or is not readable.
> +
>  Bugs fixed in 1.11.0a:
>  
>  * Bugs introduced by 1.11:
> diff --git a/aclocal.in b/aclocal.in
> index 2210fe3..3b9beab 100644
> --- a/aclocal.in
> +++ b/aclocal.in
> @@ -312,7 +312,15 @@ sub scan_m4_dirs ($@)
>      {
>        if (! opendir (DIR, $m4dir))
>       {
> -       fatal "couldn't open directory `$m4dir': $!";
> +       if ($type == FT_USER)
> +         {
> +           verb "cannot open directory `$m4dir': $!";
> +           next;
> +         }
> +       else
> +         {
> +           fatal "couldn't open directory `$m4dir': $!";
> +         }
>       }
>  
>        # We reverse the directory contents so that foo2.m4 gets

> --- /dev/null
> +++ b/tests/aclocal-bad-dirlist-include-dir.test

Wouldn't "bad-dirlist-include" be a long-enough and precise-enough name?

> +# 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

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

> +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'?).

> +cat > configure.in <<END
> +AC_INIT([$me], [1.0])
> +MY_MACRO
> +NOT_A_MACRO
> +END
> +
> +mkdir m4
> +cat > m4/my-defs.m4 <<END
> +AC_DEFUN([MY_MACRO], [my--macro--expansion])
> +END
> +
> +mkdir unreadable unopenable unopenable/sub private
> +
> +cat > unreadable/not-defs.m4 <<END
> +AC_DEFUN([NOT_A_MACRO], [invalid--expansion])
> +END
> +cp unreadable/not-defs.m4 unopenable/sub
> +cp unreadable/not-defs.m4 private
> +
> +chmod a-r unreadable
> +chmod a-x unopenable
> +chmod a-rwx private
> +
> +ls -l unreadable && Exit 77
> +ls -l unopenable/sub && Exit 77
> +ls -l private && Exit 77
> +
> +aclocal_call ()
> +{
> +  $ACLOCAL -I m4 \
> +           -I non-existent \
> +           -I "$cwd"/non-existent \
> +           -I unreadable \
> +           -I "$cwd"/unreadable \
> +           -I unopenable/sub \
> +           -I "$cwd"/unopenable/sub \
> +           -I private \
> +           -I "$cwd"/private \
> +           ${1+"$@"} >stdout 2>stderr
> +}
> +
> +cwd=`pwd` || Exit 99
> +
> +aclocal_call && test ! -s stdout && test ! -s stderr \
> +  || { cat stdout; cat stderr >&2; Exit 1; }
> +
> +$EGREP '(MY_MACRO|my-defs\.m4)' aclocal.m4
> +$EGREP '(NOT_A_MACRO|not-defs\.m4)' aclocal.m4 && Exit 1
> +
> +$AUTOCONF
> +
> +$FGREP MY_MACRO configure && Exit 1
> +$FGREP my--macro--expansion configure
> +$FGREP NOT_A_MACRO configure
> +$FGREP invalid--expansion configure && Exit 1
> +
> +aclocal_call --verbose || { cat stdout; cat stderr >&2; Exit 1; }
> +cat stdout
> +cat stderr >&2
> +
> +for d in non-existent unreadable unopenable/sub private; do
> +  grep " cannot open .*$d" stderr
> +  grep " cannot open .*$cwd/$d" stderr
> +done

> --- /dev/null
> +++ b/tests/aclocal-bad-system-include-dir.test

> +# Check that `aclocal' errors out when passed a non-existent or
> +# non-readable directory with the `dirlist' file.

> +cat > configure.in <<END
> +AC_INIT([$me], [1.0])
> +END
> +
> +mkdir fake-acdir
> +chmod a-r fake-acdir
> +ls -l fake-acdir && Exit 77
> +
> +$ACLOCAL --acdir fake-acdir 2>stderr && { cat stderr >&2; Exit 1; }
> +cat stderr >&2
> +grep " couldn't open directory .*fake-acdir" stderr



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

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

> +. ./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
> +  *$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

> +# 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?  Can't hurt to also test that it fails, no?

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

> --- /dev/null
> +++ b/tests/aclocal-verbose-install.test

> +# Check verbose messages by `aclocal --install'.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat > configure.in <<END
> +AC_INIT([$me], [1.0])
> +MY_MACRO_BAR
> +MY_MACRO_QUUX
> +END
> +
> +mkdir dirlist-test
> +cat > dirlist-test/bar.m4 <<END
> +AC_DEFUN([MY_MACRO_BAR], [:])
> +END
> +cat > dirlist-test/quux.m4 <<END
> +AC_DEFUN([MY_MACRO_QUUX], [:])
> +END
> +
> +mkdir foodir
> +: > foodir/bar.m4
> +
> +ACLOCAL_TESTSUITE_FLAGS='-I foodir' $ACLOCAL --install --verbose 2>stderr \
> + || { cat stderr >&2; Exit 1; }
> +cat stderr >&2
> +grep ' installing .*dirlist-test/bar\.m4.* to .*foodir/bar\.m4' stderr
> +grep ' installing .*dirlist-test/quux\.m4.* to .*foodir/quux\.m4' stderr
> +grep ' overwriting .*foodir/bar\.m4.* with .*dirlist-test/bar\.m4' stderr
> +grep ' installing .*foodir/quux\.m4.* from .*dirlist-test/quux\.m4' stderr
> +
> +# Sanity checks.
> +ls -l foodir
> +grep MY_MACRO_BAR foodir/bar.m4
> +grep MY_MACRO_QUUX foodir/quux.m4