On Monday 14 February 2011, Ralf Wildenhues wrote: > Hi Stefano, > > a while ago ... > > * Stefano Lattarini wrote on Tue, Jan 25, 2011 at 06:38:50PM CET: > > <http://lists.gnu.org/archive/html/automake-patches/2010-11/msg00152.html> > > <http://lists.gnu.org/archive/html/automake-patches/2010-12/msg00006.html> > > > OK for master? > > Both are OK, with nits addressed. > > Thanks, > Ralf > > > Subject: [PATCH 1/2] tests: optimize `instspc-*.test' for speed > > > > After the split of `instspc.test' into various generated tests, > > the running time of the testsuite has noticeably increased, since > > all these new generated tests must run aclocal, autoconf and > > automake, whereas previously they were run only once (at the > > beginning of `instspc.test'). But luckily, since the new tests > > share the same input files for the autotools, this situation can > > be easily worked around (at the expenses of a slight increase of > > complexity for the testsuite scaffolding). > > > > * tests/instspc-data.test: New helper test, properly calling > > the `instspc-tests.sh' script to generate input data for the > > others `instspc-*.test' tests. > > * tests/Makefile.am (TESTS): Add `instspc-data.test'. > > ($(instspc_tests:.test=.log)): Depend on its log file. > > (instspc-data.log): Depend on `instspc-tests.sh'. > > * tests/instspc-tests.sh: Recognize new action `generate-data', > > and use it to create hand-written and autotools-generated static > > files shared by all the `instspc-*.test' tests. > > When sourced by the `instspc-*.test' tests, use those previously > > created files instead of recreating them from scratch. > > (deindent, create_input_data): New subroutines. > > Some other related changes and refactorings. > > > [CUT] > > > --- /dev/null > > +++ b/tests/instspc-data.test > > > +# Helper testcase which generate input data for the other test > > +# `instspc-*.test'. It basically delegates the work to the helper > > +# script `instspc-test.sh'. > > As an alternative to a helper testcase, this could also just be a helper > script whose run is a prerequisite to the instspc*.log files. That way > you don't have a bogus test result. E.g.: > > $(instspc_tests:.test=.log): instspc-tests.sh instspc-data.dir/.dirstamp > instspc-data.dir/.dirstamp: > srcdir=$(srcdir) $(SHELL) $(srcdir)/tests/instspc-setup > > or so (with the script renamed to instspc-setup). And mostlyclean-local > removing instspc-data.dir. > > You decide whether this is worthwhile. > I had already tried a similar approach in the first version of the patch: <http://lists.gnu.org/archive/html/automake-patches/2010-11/msg00152.html> but IMHO that turned out to be slightly more fragile and more complex than the current approach. So I'd rather not go back there.
> > +# Ensure proper definition of $testsrcdir. > > +. ./defs-static || exit 99 > > +test -n "$testsrcdir" || exit 99 # sanity check > > + > > +instspc_action=generate-data > > +. $testsrcdir/instspc-tests.sh > > > --- a/tests/instspc-tests.sh > > +++ b/tests/instspc-tests.sh > > > @@ -94,6 +99,91 @@ define_problematic_string () > > esac > > } > > > > +# Helper subroutines for creation of input data files. > > + > > +deindent () > > "unindent"? Hmm. Both sound weird, but maybe unindent is easier to > read. > They're both fine with me, so I went for `unindent'. > Might wanna have it in tests/defs? > Hmmm... maybe in a follow-up patch. But then, IMHO, we would want a smarter implementation, that doesn't undiscriminately strip away any leading whitespace from every line. Maybe it should look at the first non-blank line to determine which amount of whitespace to remove. I.e., something like: deindent > main.c <<EOF main() { return 0; } EOF should result in main.c containing: main() { return 0; } WDYT? > > [CUT] > > > > @@ -189,102 +279,54 @@ if test x"$instspc_action" = x"generate-makefile"; > > then > > exit 0 > > fi > > > > -### If we are still here, we have to run a test ... > > - > > -# We'll need the full setup provided by `tests/defs'. Temporarly disable > > +# We'll need the full setup provided by `tests/defs'. Temporarily disable > > # the errexit flag, since the setup code might not be prepared to deal > > # with it. > > set +e > > . ./defs || Exit 99 > > set -e > > > > +# The directory set up by the `generate-data' action should contain all > > +# the files we need. So remove the other files created by ./defs. And > > +# check we really are in a temporary `*.dir' directory in the build tree, > > +# since the last thing we want is to remove some random user files! > > +test -f ../defs-static && test -f ../defs || Exit 99 > > +case `pwd` in *.dir);; *) Exit 99;; esac > > +rm -f * > > + > > +if test x"$instspc_action" = x"generate-data"; then > > + # We must *not* remove the test directory, since its contents must be > > + # used by following dependent tests. > > + trap 'Exit $?' 0 > > trap '' 0 > is the portable way to undo the EXIT trap, if I remember correctly. > You could also just set keep_testdirs, no? > Yes, that's cleaner and clearer. Thanks for the suggestion, I've amended the patch accordingly. > > + create_input_data > > + Exit 0 > > +fi > > > Subject: [PATCH 2/2] tests: `instspc-*.test': do not create useless source > > file > > > > * tests/instspc-tests.sh (create_input_data): Do not create > > unused source file `source2.c'. > I have pushed both the patches to master now. Thanks, Stefano