Re: [PATCH] {yacc-work} configure: look for a lex program to be used by the testsuite

[Stefano Lattarini]

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