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

[Ralf Wildenhues]

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

Thanks,
Ralf

> --- /dev/null
> +++ b/tests/autodist-aclocal-m4.test

> +# Check that `aclocal.m4' is not automatically distributed if nor
> +# required to build `configure'.

s/ if//  ?

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

> +# Related to automake bug#7819.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +{ echo 'm4_include([defs.m4])'
> +  cat configure.in
> +  echo 'AC_OUTPUT'
> +} > t
> +mv -f t configure.in
> +
> +cat > Makefile.am <<'END'
> +.PHONY: test
> +test: distdir
> +     ls -l $(distdir)
> +     test ! -f $(distdir)/aclocal.m4
> +     echo $(DISTFILES) | grep 'aclocal\.m4' && exit 1; :
> +     echo $(DIST_COMMON) | grep 'aclocal\.m4' && exit 1; :
> +check-local: test
> +END
> +
> +: > defs.m4
> +$ACLOCAL
> +mv -f aclocal.m4 defs.m4
> +
> +$AUTOMAKE
> +$AUTOCONF
> +
> +./configure
> +$MAKE test
> +$MAKE distcheck
> +
> +:

> --- /dev/null
> +++ b/tests/autodist-config-headers.test

> +# Check that config.h.bot and config.h.top are automatically
> +# distributed if the AC_CONFIG_HADERS macro is used, and they
> +# exist at automake runtime.

Ohh, you're gonna call me nitpicky this time.  Please remove the comma,
otherwise the sentence parses for me as "check that ... they exist at
automake runtime".

> +# Related to automake bug#7819.

> --- /dev/null
> +++ b/tests/autodist-configure-no-subdir.test

> +# Check that `configure', `configure.ac' and `configure.in' are *not*
> +# automatically distributed when placed in a subdirectory.
> +# Related to automake bug#7819.

Ah, so we are actually a bit saner than we document us to me.  Good.

> --- /dev/null
> +++ b/tests/autodist-stamp-vti.test

> +# Check that `stamp-vti' is automatically distributed when info_TEXINFOS
> +# and version.texi are involved.
> +# Related to automake bug#7819.

Saner, too.

> --- /dev/null
> +++ b/tests/autodist-subdir.test

> +# Check that automake really distributes automatically all the files

automatically distributes

> +# it is advertised to, even when in subdirectories.

it advertises to do

> +#
> +# This behavior might be suboptimal, but it has been in place for quite
> +# a long time, and it would be risky to change it now.  See also the
> +# discussion of automake bug#7819:
> +#  <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7819>
> +#
> +# Keep this test in sync with sister test `autodist.test'.

Ah, so you're just moving the "not so sane" semantics here.

> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in <<'END'
> +AC_CONFIG_FILES([sub/Makefile])
> +AC_OUTPUT
> +END
> +
> +$ACLOCAL
> +$AUTOCONF
> +
> +# 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.

> +            ;;
> +          aclocal.m4)
> +            # This file should be distributed only when it is a real
> +            # deprendency for configure.  Anyway, not a check to be

dependency

> +            # performed in this test.
> +            ;;
> +          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?  Remember, people are not computers, they do not like to read
cut and pasted text.

> +            ;;
> +          *)
> +            echo $f
> +            ;;
> +        esac
> +      done`
> +# Normalize whitespaces, just in case.

whitespace
(This is another one of those words that never exist in plural form,
unlike spaces which do exist.)

> +list=`echo $list`
> +
> +test -n "$list" || Exit 1

That `|| Exit 1' is not needed, right?  ;-)

> +cat > Makefile.am <<'END'
> +SUBDIRS = sub
> +check-local:
> +## For debugging.
> +     @echo DIST_COMMON:
> +     @for f in $(DIST_COMMON); do echo "  $$f"; done
> +     @echo DISTDIR:
> +     @ls -l $(distdir) | sed 's/^/  /'
> +## Now the checks.
> +     @for f in $(autodist_list); do \
> +       echo "file: sub/$$f"; \
> +       test -f $(distdir)/sub/$$f \
> +         || { echo $$f: distdir fail >&2; exit 1; }; \
> +     done
> +END
> +
> +mkdir sub
> +
> +cat > sub/Makefile.am <<'END'
> +include distfiles.am
> +check-local:
> +## For debugging.
> +     @echo DIST_COMMON:
> +     @for f in $(DIST_COMMON); do echo "  $$f"; done
> +     @echo DISTDIR:
> +     @ls -l $(distdir) | sed 's/^/  /'
> +## 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.

> +       ## low-probability event that it's not worth worrying about.
> +       echo ' ' $(DIST_COMMON) ' ' | grep "[ /]$$f " >/dev/null \
> +         || { echo $$f: distcom fail >&2; exit 1; }; \
> +     done
> +END
> +
> +: First try listing the automatically-distributed files in proper
> +: targets in Makefile.am
> +
> +echo "MAINTAINERCLEANFILES = $list" > sub/distfiles.am
> +for f in $list; do echo "$f:; touch $f"; done >> sub/distfiles.am
> +
> +cat sub/distfiles.am # For debugging.
> +
> +$AUTOMAKE -a
> +
> +./configure
> +
> +$MAKE distdir
> +autodist_list="$list" $MAKE check
> +
> +$MAKE maintainer-clean
> +test ! -f sub/README    # Sanity check.
> +rm -rf $me-1.0          # Remove $(distdir).
> +
> +: Now try creating the automatically-distributed files before
> +: runnng automake.

running

> +: > sub/distfiles.am
> +for f in $list; do
> +  echo dummy > sub/$f
> +done
> +
> +ls -l # For debugging.
> +
> +$AUTOMAKE
> +
> +./configure
> +
> +$MAKE distdir
> +autodist_list="$list" $MAKE check

> --- /dev/null
> +++ b/tests/autodist.test

> +# Check that automake really distributes automatically all the files

see above.  Here and with lots of stuff below

> +# it is advertised to.

see above

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


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

> +done >> distfiles.am
> +
> +cat distfiles.am # For debugging.
> +
> +$AUTOMAKE -a
> +
> +./configure
> +
> +$MAKE distdir
> +autodist_list="$list" $MAKE check
> +
> +$MAKE maintainer-clean
> +test ! -f README        # Sanity check.
> +rm -rf $me-1.0          # Remove $(distdir).
> +
> +: Now try creating the automatically-distributed files before
> +: runnng automake.

running

> +: > distfiles.am
> +for f in $list; do
> +  echo dummy > $f
> +done
> +
> +ls -l # For debugging.
> +
> +$AUTOMAKE
> +
> +./configure
> +
> +$MAKE distdir
> +autodist_list="$list" $MAKE check