On Tuesday 11 January 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Tue, Jan 11, 2011 at 12:05:20AM CET:
> > tests: add checks on automatically-distributed files
> >
> > Related to automake bug#7819.
> >
> > * tests/autodist.test: New test.
> > * tests/autodist-subdir.test: Likewise.
> > * tests/autodist-acconfig.test: Likewise.
> > * tests/autodist-acconfig-no-subdir.test: Likewise.
> > * tests/autodist-aclocal-m4.test: Likewise.
> > * tests/autodist-config-headers.test: Likewise.
> > * tests/autodist-configure-no-subdir.test: Likewise.
> > * tests/autodist-stamp-vti.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
>
> OK with nits addressed.
>
I agree with most of your nits, and thus I fixed most of them.
My remarks and/or objections to other nits are inline below.
I've pushed the patch to maint.
Thanks,
Stefano
-*-*-
> > --- /dev/null
> > +++ b/tests/autodist-aclocal-m4.test
>
> > +# Check that `aclocal.m4' is not automatically distributed if nor
> > +# required to build `configure'.
>
> s/ if// ?
>
Nope, s/nor/not/.
> But wait. We do not actually require this to work, no? Where is it
> documented? I think that this works is only by accident (e.g.,
> "undefined behavior").
>
I somewhat agree, but still, the behaviour makes sense, so it's nice to
test it explicitly and ensure we don't break it by accident. Anyway,
after your observation, I think that a better heading comment is in
order here; I went for this:
# Check that `aclocal.m4' is not automatically distributed if not
# required to build `configure'. This is *really* a corner-case
# check, and the behaviour it checks is not documented either, so
# if that behaviour is deliberately changed in the future, just
# remove this test.
>
> > --- /dev/null
> > +++ b/tests/autodist-config-headers.test
>
> > +# The automake manual tells that the list of automatically-distributed
> > +# files should be given by `automake --help'.
> > +list=`$AUTOMAKE --help \
> > + | sed -n '/^Files .*automatically distributed.*if found/,/^ *$/p' \
> > + | sed 1d`
> > +list=`for f in $list; do
> > + case $f in
>
> trailing space
>
> > + configure|configure.in|configure.ac)
> > + # The files configure.ac, configure.in and configure are
> > + # special-cased enough that we want to meddle with them.
>
> The logic in this sentence is wrong.
>
Yeah, a comment like "see test autodist-configure-no-subdir.test" should
be better. Fixed.
> > + acconfig.h)
> > + # Works only when it really exists, not when it is a
> > + # target in Makefile.am; see also automake bug#7819.
> > + ;;
> > + stamp-vti)
> > + # Works only when using info_TEXINFOS and version.texi;
> > + # see also automake bug#7819.
> > + ;;
> > + config.h.bot|config.h.top)
> > + # Works only when the AC_CONFIG_HADERS macro is used;
> > + # see also automake bug#7819.
>
> Do you really think it is helpful to reference the same bug three times
> within ten lines of text, when it is already given in the header
> comment?
>
Honestly, I didn't pay much attention to this, as the above munging
of $list should go away once bug#7819 is fixed (which I hope will
happen soon). Anyway, I've removed the extra "copy&paste" references
to that bug, which shouldn't hurt either.
> > +## Now the checks.
> > + @for f in $(autodist_list); do \
> > + echo "file: $$f"; \
> > + ## Some filenames might contains dots, which are metacharacters
> > + ## for grep. But this won't cause spurious failures, and the
> > + ## evantuality of a "spurious success" caused by them is such a
>
> contain
> eventuality
>
> Why not just:
> ## Some filenames might contain dots.
>
I went for this "middle-ground" formulation instead:
## Some filenames might contain dots, but this won't cause spurious
## failures, and "spurious successes" are so unlikely that they're
## not worth worrying about.
I hope that's ok with you.
> > + echo ' ' $(DIST_COMMON) ' ' | grep "[ /]$$f " >/dev/null \
> > + || { echo $$f: distcom fail >&2; exit 1; }; \
> > + done
> > +END
> > +
> > --- /dev/null
> > +++ b/tests/autodist.test
>
> > +# Related to automake bug#7819.
> > +# Keep this test in sync with sister test `autodist-subdir.test'.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +# Ensure we are run from the right directory.
> > +# (The last thing we want is to delete some random user files.)
> > +test -f ../defs-static
> > +rm -f *
>
> Ouch. How about just removing what you know and want to remove?
>
That's a copy-and-paste from another test, so it would be better to
fix all similar usages in a follow-up patch IMO. Maybe a new "option"
for ./defs which prevents the creation of files in the TESTNAME.dir
temporary directory would be useful. But see (just) below.
> This can be a real concern: this may be on w32 or an NFS mount,
> removing files held open (by, say, my editor or tee logfile ;-) may
> fail hard and abort the test.
>
But the test directory those files are into would have been just
removed and re-created from scratch, so, if there were open files
in the old test directory, the testcase would have already aborted,
right? Or am I missing something?
> > +echo "MAINTAINERCLEANFILES = $list" > distfiles.am
> > +for f in $list; do
> > + # This indirections are needed to avoid redefining automake-generated
>
> These
>
> > + # targets such as aclocal.m4.
> > + echo "$f: touch-$f"
> > + echo "touch-$f:; touch $f"
>
> Please leave a space after the colon (even when it otherwise looks
> ugly to leave a space before a semi-colon). This makes it easier
> to detect that this is a rule with commands. Thanks.
>
I will.
BTW, I notice now that these indirections aren't really needed (they are
a leftover of an older version of the patch), so I just removed them as
follows:
echo "MAINTAINERCLEANFILES = $list" > distfiles.am
for f in $list; do echo "$f :; touch $f"; done >> distfiles.am
-*-*-
Regards,
Stefano
