Re: [PATCH] {maint} tests: add checks on automatically-distributed files

[Stefano Lattarini]

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