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