On Saturday 29 January 2011, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Fri, Jan 28, 2011 at 11:19:12PM CET: > > This will allow the testcases requiring a 'lex' program to run also > > with vendor/legacy lex implementations, not only with 'flex'. > > > * configure.ac: Look for a lex program, using AC_CHECK_PROGS. > > * tests/defs.in: New required entry 'lex'. > > ($LEX): Let the user override the lex program to be used by the > > testsuite. > > * tests/cond35.test ($required): Require 'lex', not 'flex'. > > * tests/cond36.test: Likewise. > > * tests/lexv3.test: Likewise. > > * tests/lexv3.test: Likewise. > > * tests/silent-lex-gcc.test: Likewise. > > * tests/silent-lex-generic.test: Likewise. > > * tests/silent-many-gcc.test: Likewise. > > * tests/silent-many-generic.test:likewise. > > * tests/lexvpath.test: Likewise, and fix typo in comments. > > > OK for the 'yacc-work' branch? (Branch which, at this point should > > probably be renamed to 'lex-yacc-work' ...) > > OK with nits addressed. > I'll wait for further feedback, because I've some doubts, and some nits of my own. Sorry about that.
> The branch name is fine as it is, > no need to strive for branch name perfection. > OK. > > --- a/configure.ac > > +++ b/configure.ac > > @@ -103,6 +103,16 @@ AC_CHECK_PROG([TEX], [tex], [tex]) > > # configure help screen. > > AC_CHECK_PROGS([YACC], [yacc byacc 'bison -y'], [false]) > > > > +# The test suite will skip some tests if no yacc program is available. > > +# We don't use AC_PROG_LEX because: > > +# 1. we don't want flex to be preferred to system lex; > > +# 2. we don't want $LEX to be defined to ':' by default; > > +# 3. we prefer not to have the LEXLIB and LEX_OUTPUT_ROOT variables > > +# to be calculated and/or AC_SUBST'd; > > +# 4. we prefer that the LEX variable is not reported in the > > +# configure help screen. > > You need to withstand copy and paste. This comment is copied and pasted > from a few lines above. It is erroneous (search for 'yacc') and immoral > (towards a reader of the code) to do so. Please find a formulation and > one comment that applies to both AC_CHECK_PROGS lines that can then come > right after one another. > > Resist copy and paste! > What about the following? # The test suite will have to skip some tests if no lex or yacc # program is available. # We don't use AC_PROG_LEX nor AC_PROG_YACC here because: # 1. we don't want flex (resp. bison) to be preferred to system lex # (resp. system yacc); # 2. we don't want $LEX (resp. $YACC) to be defined to ':' (resp. 'yacc') # by default; # 3. we prefer not to have the variables YFLAGS, LEX_OUTPUT_ROOT and # LEXLIB to be calculated and/or AC_SUBST'd; # 4. we prefer that the YACC and LEX variables are not reported in the # configure help screen. AC_CHECK_PROGS([YACC], [yacc byacc 'bison -y'], [false]) AC_CHECK_PROGS([LEX], [lex flex], [false]) > > +AC_CHECK_PROGS([LEX], [lex flex], [false]) > > + > > # Generate man pages. > > AM_MISSING_PROG([HELP2MAN], [help2man]) > > > --- a/tests/defs.in > > +++ b/tests/defs.in > > @@ -63,6 +63,7 @@ export SHELL > > # User can override various tools used. > > test -z "$PERL" && PERL='@PERL@' > > test -z "$YACC" && YACC='@YACC@' > > +test -z "$LEX" && LEX='@LEX@' > > test -z "$MAKE" && MAKE=make > > test -z "$AUTOCONF" && AUTOCONF="@am_AUTOCONF@" > > test -z "$AUTOHEADER" && AUTOHEADER="@am_AUTOHEADER@" > > @@ -210,6 +211,17 @@ do > > echo "$me: running texi2dvi -o /dev/null --version" > > ( texi2dvi -o /dev/null --version ) || exit 77 > > ;; > > + lex) > > + if test x"$LEX" = x"false"; then > > + # No lex program was found at configure time, or the user has > > + # explicitly told he doesn't want a lex program to be used. > > + echo "$me: \$LEX is \"false\", skipping test" >&2 > > That error message could be more helpful: > $me: no working \$LEX found at configure time, or disabled > > obviating the need for the comment too. > I agree. But since there's also a similar pre-existing suboptimal message for the 'yacc' prerequisite, couldn't I fix them both in a follow-up patch instead? > > + exit 77 > > + else > > + # Make LEX available to configure by exporting it. > > Superfluous comment (the code already shows what you do). > But it doesn't shows why; maybe using just: # Make LEX available to configure. or: # Pick up LEX for configure. would be better? > > + export LEX > > + fi > > + ;; > > yacc) > > if test x"$YACC" = x"false"; then > > # No yacc program was found at configure time, or the user has > > I haven't checked the individual tests that you changed, assuming that > you tested them with a non-flex lex program on some system. > Yes, I've tested with: $ LEX=/opt/heirloom/bin/lex make check TESTS="`echo lex*.test`" Regards, Stefano