Bug 20253 – A correct Webkit build requires flex 2.5.33 but autotools build doesn't check for it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Kalle Vahlman]

Currently, the autotools configure.ac only checks that flex is present but doesn't verify that it is at least the required 2.5.33 version.

The big problem with this is that flex 2.5.31 is able to produce correct enough code for it to build successfully. However, the incorrect version presents itself (at least) in various CSS-directives failing to work properly (eg. colors).

This is a very subtle problem that is very hard to debug, and it took me about a week to traverse from "why does this web page look wrong" to "oh, CSS parsing results in totally bogus colors" to "oh! Wrong version of flex causes it!"

Thus, check for the version is very important for the sanity of people building for example with a Scratchbox environment (which has .31 in its current official release).

[Kalle Vahlman]

Created attachment 22597 [details]
Add a check for recent enough flex version to configure.ac

Here's a patch to add a check for flex version.

Unfortunately there is no sensible way to check the version, so it needs to be done through sed magic.

[David Kilzer (:ddkilzer)]

Comment on attachment 22597 [details]
Add a check for recent enough flex version to configure.ac

Thanks for the patch, Kalle!  Please set the "review?" flag if you want the patch to be reviewed for committing.  It should also have a ChangeLog entry:

http://webkit.org/coding/contributing.html

Thanks again!

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=22597) [edit]
> Add a check for recent enough flex version to configure.ac
> 
> Here's a patch to add a check for flex version.
> 
> Unfortunately there is no sensible way to check the version, so it needs to be
> done through sed magic.

Why can't the "PKG_CHECK_MODULES" macro be used as is done elsewhere in configure.ac?

[Eric Seidel (no email)]

Comment on attachment 22597 [details]
Add a check for recent enough flex version to configure.ac

This seems very complicated.  Are we sure there is no simpler way to do this check in configure.ac?

I did a little google searching:
http://archives.postgresql.org/pgsql-patches/2003-07/msg00019.php
http://cvs.tuxbox.org/lists/tuxbox-cvs-0505/msg00086.html
https://gna.org/bugs/?11627

I'm sure there are more pages...

r- until David's question and the above complexity question are answered.

Thanks for the patch!  Looking forward to hearing your reply.

[Kalle Vahlman]

(In reply to comment #3)
> Why can't the "PKG_CHECK_MODULES" macro be used as is done elsewhere in
> configure.ac?
 
That macro requires the checked software to install a .pc file for pkg-config to find, and that practice is not too widespread outside the GNOME/freedesktop.org circles... Which is a bit unfortunate since it would make life so much simpler.

About the complicated approach, well yes, I suppose I went a bit overboard to provide easy update should the requirement ever change :) It's not like flex would have had even minor version bump in the last decade...

I'll redo the test so that it only looks at the micro version directly, like the PostgreSQL does. And create a proper patch this time.

[Kalle Vahlman]

Created attachment 22615 [details]
Version 2 of flex checking

So here is a simplified version check, only looking at the micro version, and a proper changelog entry.

[Eric Seidel (no email)]

Comment on attachment 22615 [details]
Version 2 of flex checking

Won't this check break with Flex 2.6?  Geez autoconf syntax sucks.

This patch would make our behavior with Flex 2.6 worse than our current behavior.

[Kalle Vahlman]

(In reply to comment #7)
> (From update of attachment 22615 [details] [edit])
> Won't this check break with Flex 2.6?  Geez autoconf syntax sucks.

Well, it's more the tools that don't provide any way to check their version in a sensible way than autoconf itself being broken, but the situation sucks indeed.

> This patch would make our behavior with Flex 2.6 worse than our current
> behavior.
 
Future-proofing the test would mean testing the major/minor/micro numbers separately and hence the original more complex version. :)

Note that I wouldn't hold my breath for a 2.6 release as flex has really been in the 2.5 series for over 10 (maybe even 15) years. I would imagine 2.6 would require incompatible changes to be introduced, and thus the check would need to exclude it anyway.

I don't care that much how the check is formulated, but I just want to prevent people running into this very sneaky problem.

So, what should we do? Bring back the uglier, but future-proof version or..?

I'm not familiar with the actual usage of flex, maybe there's some way to check for version at build time?

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (In reply to comment #3)
> > Why can't the "PKG_CHECK_MODULES" macro be used as is done elsewhere in
> > configure.ac?
> 
> That macro requires the checked software to install a .pc file for pkg-config
> to find, and that practice is not too widespread outside the
> GNOME/freedesktop.org circles... Which is a bit unfortunate since it would make
> life so much simpler.

That means that none of the other version checks in the current configure.ac file work for your build system?!

Is there a way to provide a decent fall-back mechanism if PKG_CHECK_MODULES doesn't have any .pc files to fall back on?

I'd rather see the more complex test than the simple one, given the choice.  Is there a "version check macro" available that is smart enough to compare multi-dot version numbers (like 2.4 <= 2.5.33 <= 2.6) rather than rolling our own regular expressions?

[Kalle Vahlman]

(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Why can't the "PKG_CHECK_MODULES" macro be used as is done elsewhere in
> > > configure.ac?
> > 
> > That macro requires the checked software to install a .pc file for pkg-config
> > to find, and that practice is not too widespread outside the
> > GNOME/freedesktop.org circles... Which is a bit unfortunate since it would make
> > life so much simpler.
> 
> That means that none of the other version checks in the current configure.ac
> file work for your build system?!

No no, it just means that flex doesn't install a .pc file so pkg-config cannot be used to detect it. Things like GStreamer and curl do install a .pc file so they can.

> I'd rather see the more complex test than the simple one, given the choice.  Is
> there a "version check macro" available that is smart enough to compare
> multi-dot version numbers (like 2.4 <= 2.5.33 <= 2.6) rather than rolling our
> own regular expressions?

That's a very good question, and the answer is yes. Not in autoconf itself though, but in the autoconf macro archive there's a macro called AX_COMPARE_VERSION that does just that:

  http://autoconf-archive.cryp.to/ax_compare_version.html

So adding this macro to acinclude.m4 (which is the proper way to include extra macros) takes us down to just one sed call (to extract the version string) and the check itself. Naturally, the macro is still a bunch of sed magic, but at least it's not in configure.ac anymore...

I'll cook up a patch for that too.

[Kalle Vahlman]

Created attachment 22621 [details]
Take 3 on flex checking

[David Kilzer (:ddkilzer)]

Comment on attachment 22621 [details]
Take 3 on flex checking

>+   FLEX_VERSION=`$FLEX --version | sed 's,.*\ \([0-9]*\.[0-9]*\.[0-9]*\)$,\1,'`
>+   AX_COMPARE_VERSION([2.5.33],[gt],[$FLEX_VERSION],
>+      AC_MSG_ERROR([You need at least version 2.5.33 of the 'flex' lexer generator to compile WebKit]))
> fi

Shouldn't that be "[ge]" since 2.5.33 works?

[Kalle Vahlman]

(In reply to comment #12)
> (From update of attachment 22621 [details] [edit])
> >+   FLEX_VERSION=`$FLEX --version | sed 's,.*\ \([0-9]*\.[0-9]*\.[0-9]*\)$,\1,'`
> >+   AX_COMPARE_VERSION([2.5.33],[gt],[$FLEX_VERSION],
> >+      AC_MSG_ERROR([You need at least version 2.5.33 of the 'flex' lexer generator to compile WebKit]))
> > fi
> 
> Shouldn't that be "[ge]" since 2.5.33 works?

No, the statement is "if 2.5.33 is greater than $FLEX_VERSION, then AC_MSG_ERROR, else do nothing" so anything starting from 2.5.33 goes to the (omitted) false-branch and continues on.

One could change it around by adding an empty true-branch there:

AX_COMPARE_VERSION([2.5.33],[ge],[$FLEX_VERSION],,
  AC_MSG_ERROR([...]))

but that looks more like a typo then expected syntax... adding empty quotation might help, but

AX_COMPARE_VERSION([2.5.33],[ge],[$FLEX_VERSION], [],
  AC_MSG_ERROR([...]))

doesn't look any better to me.

I can change it of course, if the positive test is deemed better.

[David Kilzer (:ddkilzer)]

Comment on attachment 22621 [details]
Take 3 on flex checking

Thanks for the explanation!  I like it the way it is.  r=me

[Jan Alonzo]

landed in r35622