On Thursday 16 September 2010, Ralf Wildenhues wrote: > Hi Stefano, Hi Ralf. Thanks for persisting on this.
> * Stefano Lattarini wrote on Thu, Sep 16, 2010 at 04:24:46AM CEST: > > Your objections, reasonings and suggestsions have been quite > > enlighting, and brought me to the understandanding that most of > > our problems were deriving by the juggling of "weird strings" > > ;-> between gen-instspc-tests, instspc.sh and the generated > > Makefile.in. Thus I've attempted a rewrite of the patch, where > > names and contents of the problematic strings are kept in a > > single, centralized place: a new "driver script" > > `instspc-tests.sh'. > > Cool. > > With nits below addressed, the patch is OK for master, but please > commit to a new branch off of maint That's what I tried at first, but unfortunately the maint branch lacks support for silent-rules. However, it might be possible that $(AM_V_GEN) and friends behaves as no-op in maint, so I might be able to do the rebase anyway. If I succeeds, I'll post the updated patch as a "FYI". > and merge that to master (that way, I hope we may have an easier time > doing merge fixups from other testsuite changes later). Thanks. > > I have an idea for a followup patch by the way: have a prerequisite > of all the instspc-*test logs create instspc.dir/ with all the > files in it, and autotools already run. Then let each individual > test create a build directory in its own test directory, but reuse > the source tree ../../instspc.dir/configure [...] > $MAKE ... > > that way you can regain almost all of the newly-introduced overhead > by not running aclocal, autoconf, automake, more than once. I was starting to develop a roughly similar idea :-) But as you said, that's for a follow-up patch. > On my system, the overhead amounts to going from 1:20 to 3:48 (this > is just the instspc* tests), which amounts to a net increase of the > whole testsuite by, I'm guessing, 15%. Yes, keeping the modularity without the performance penalty would definitely be a boon. Thinking about it, your proposal would in fact even *increase* the modularity! > > --- a/ChangeLog > > +++ b/ChangeLog > > @@ -1,4 +1,41 @@ > > -2010-09-14 Stefano Lattarini <stefano.lattar...@gmail.com> > > +2010-09-16 Stefano Lattarini <stefano.lattar...@gmail.com> > > This looks fishy, you seem to be modifying the date of a previous > commit entry, rather than adding a new one. Botched amending; fixed. > > + * tests/instspc-tests.sh: New script, fullfilling a double role: > fulfilling Fixed. > > + code and test execution code are inevitably interwined. > intertwined Ditto. > > --- a/tests/Makefile.am > > +++ b/tests/Makefile.am > > +$(instspc_tests): Makefile.am > > + $(AM_V_at)rm -f $@ $...@-t > > + $(AM_V_GEN) :; \ > > + base=`expr x'$@' : x'instspc-\(.*\)\.test$$'`; \ > > I think $@ could contain $(srcdir) here, no? Why? In "True VPATH Spirit", we generate tests in the builddir, not in the srcdir. > You could just kill off all directory components to be sure. I haven't done this yet ATM. But if you still think it will be safer, I'll follow your judgement and do the amending. > [...] > > > --- /dev/null > > +++ b/tests/instspc-tests.sh > > +# This script fullfills a double role: > fulfills Fixed. > Alternatively, you could have split the thing in three: a generator > script, a test run script, and the common parts in a scriplet > sourced by both. I'm OK with the way it is now, though. OK, thanks. I'd really like to avoid another respinning, if not absolutely necessary. > > +# 1. It generates a Makefile.am snippet, containing the definintion > definition Fixed. > > +# the test generation code and test execution code tend to be > > +# inextricably coupled and interwined. > intertwined Ditto. > > +set -e > > [...] You missed this: > else > echo "$0: empty action and no proper command line " >&2 > exit 99 Extra trailing space in error message. Fixed. > > > +elif test $# -gt 0; then > > + echo "$0: action specified and command line arguments used" > > >&2 + exit 99 > > +elif test x"$instspc_action" = x"generate-makefile"; then > > > + : > indentation Oops. > > +else > > + case $instspc_action in > > + test-build|test-install) > > + if test x"$instspc_test_name" = x; then > > + echo "$0: test name undefined for action > > '$instspc_action'" >&2 + exit 99 > > + fi;; > > + *) > > + echo "$0: invalid action: '$instspc_action'" > > + exit 99;; > > + esac > > +fi > > + > > +# Helper subroutine for test data definition. > > +# Usage: define_problematic_string NAME STRING > > again, this is veery nitty, and for the testsuite I don't really > mind all that much, but the GNU style comment for this function > would be something like this: > > # define_problematic_string NAME STRING [FAILURE-MODE]... > # ------------------------------------------------------- > # Register STRING in associative array instspc__<NAME> and NAME > in # instspc_names_list. Update instspc_xfail_builds_list and # > instspc_xfail_installs_list according to FAILURE-MODEs. I'd agree with you if the function were in, say, `tests/defs.in' (once the queue of pending patches in the "tests-init" branch is flushed, you'll see how nicely commented my `run_command' function is ;-). But in the present situation, a detailed explanation of the function would be overkill IMVHO. Besides, the code in `define_problematic_string' is so ad-hoc that it's difficult to meaningfully document it, and so short that it is in fact self-explanatory. WDYT? > > > > > > +# Skip if this system doesn't support these characters in file > > names. +mkdir "./$instspc_test_string" || Exit 77 > > + > > +mkdir sub1 > > + > > +cat >> configure.in << 'EOF' > > +AC_PROG_CC > > +AC_PROG_RANLIB > > +AC_OUTPUT > > +EOF > > + > > +mkdir sub > > Collapse three mkdir calls into one, to save two (times 2 times 45) > processes? Yes, nice catch. Done. > > [...] Thanks, Stefano
