* 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
