424063 - configure script check for system sqlite 3.5.7 instead of 3.5

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Hussam Al-Tayeb]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031906 Firefox/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031906 Firefox/3.0b5pre

/usr/lib/pkgconfig/sqlite3.pc from 3.5.7 (and previous versions) says 3.5
but the following change makes mozilla check for 3.5.7 instead of 3.5
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla&file=configure.in&rev1=1.1964&rev2=1.1965&whitespace_mode=show&diff_mode=context

obviously pkg-config --modversion sqlite3 says 3.5

but sqlite --version still 3.5.7


I asked in sqlite help channel on freenode.
they suggested the configure script compiles a simple C program to print out the version number from the sqlite header.

such as:

#include <stdio.h>
#include <sqlite3.h>
 
int main(int argc, char *argv[])
{
        printf("%s\n", SQLITE_VERSION);
        return 0;
}




Reproducible: Always

Comment 1

[Mike Hommey [:glandium]]

Dammit, when will you (mozilla) stop bumping version checks in configure.in when it's not useful ! There's no problem whatsoever with people building against versions older than the one in the mozilla source tree ! And if you're concerned about security, don't forget that a lot of linux distros backport security fixes to versions in their stable releases. So except when you are using new APIs, DON'T BUMP VERSION CHECKING IN CONFIGURE.IN !

Comment 2

[Hussam Al-Tayeb]

Please cool it down Mike Hommey. And for your information, not every distribution backports security fixes in non base system packages. A lot of rolling release system distributions simply wait for the next bug fix release of packages like sqlite3.
It is not anyone's fault here.

Comment 3

[Shawn Wilsher :sdwilsh]

There's a very good reason why we want to test for an exact version - we test on a specific version.  For example, right now 3.5.7 is a topcrasher (bug 424163) for us, so we won't want anyone to build with it.

The only way I'm presently comfortable ensuring the quality of the product is to build with the same version that we are shipping with.

Comment 4

[cls]

FYI, that runtime check for the version number will fail when cross-compiling.  Try this instead:
 SQLITE_VERSION_NUMBER=3005007
 AC_MSG_CHECKING(sqlite version >= $SQLITE_VERSION)
 AC_TRY_COMPILE ( [#include <sqlite3.h>], [
    #if SQLITE_VERSION_NUMBER < $SQLITE_VERSION_NUMBER
        #error "sqlite version < $SQLITE_VERSION_NUMBER)
    #endif
 ] , [res=yes], [res=no])
 AC_MSG_RESULT([$res])
 if test "$res" != "yes"; then
    AC_MSG_ERROR([sqlite version $SQLITE_VERSION or higher required])
 fi

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> FYI, that runtime check for the version number will fail when cross-compiling. 
> Try this instead:
>  SQLITE_VERSION_NUMBER=3005007
>  AC_MSG_CHECKING(sqlite version >= $SQLITE_VERSION)
>  AC_TRY_COMPILE ( [#include <sqlite3.h>], [
>     #if SQLITE_VERSION_NUMBER < $SQLITE_VERSION_NUMBER
>         #error "sqlite version < $SQLITE_VERSION_NUMBER)
>     #endif
>  ] , [res=yes], [res=no])
>  AC_MSG_RESULT([$res])
>  if test "$res" != "yes"; then
>     AC_MSG_ERROR([sqlite version $SQLITE_VERSION or higher required])
>  fi
That should be a different bug.

Comment 6

[Mike Shaver (:shaver emeritus)]

(In reply to comment #1)
> And if you're
> concerned about security, don't forget that a lot of linux distros backport
> security fixes to versions in their stable releases. So except when you are
> using new APIs, DON'T BUMP VERSION CHECKING IN CONFIGURE.IN !

Go away.  If you don't want to build our software, please don't.  We set our configuration dependencies based on what we believe produces a product in keeping with our standards for quality, based on our testing.  That's our call to make, and yelling like a jackass doesn't change it.  When you link against the wrong sqlite, and it doesn't have fixes for bugs that we hit, and your package crashes, whose reputation do you think is at stake?  (When distributions crippled Firefox 2 performance by switching to slow-boat pango code, and then topped it off by taking the additional 10% hit by switching from static to shared, who do you think users blamed that on?  It was like pulling teeth to even get distributors to note that their switch to Pango broke some web content, let alone say "yeah, so this Firefox we're shipping is 80% slower than the upstream one at rendering pages".)

Distributions that want to pretend that one version is another can take on the burden of figuring out other ways to fit into compatibility checks, since they're invalidating the entire concept of versioning as a statement of the contents of software.  Their call, to be sure, but also their mess to deal with.

I don't think we should have --enable-system-{sqlite,cairo} at all, because of exactly this case -- people want to relax or permute the dependencies based on basically _no_ knowledge of why we chose what we chose, and we get yelled at because we don't do it for them and rub their feet.  I argued against the cairo instance, and was told "oh no, the distributors will test it thoroughly, don't worry".  I wish it felt more satisfying to be right.

Comment 7

[cls]

(In reply to comment #5)
> (In reply to comment #4)
> > FYI, that runtime check for the version number will fail when cross-compiling. 
> That should be a different bug.

How so?  The original bug report is about the pkg-config check being insufficient and suggesting using a runtime check instead of the pkg-config check.  I'm suggesting the equivalent compile-time test instead of a runtime test.

Comment 8

[Hussam Al-Tayeb]

(In reply to comment #4)
> FYI, that runtime check for the version number will fail when cross-compiling. 
> Try this instead:
>  SQLITE_VERSION_NUMBER=3005007
>  AC_MSG_CHECKING(sqlite version >= $SQLITE_VERSION)
>  AC_TRY_COMPILE ( [#include <sqlite3.h>], [
>     #if SQLITE_VERSION_NUMBER < $SQLITE_VERSION_NUMBER
>         #error "sqlite version < $SQLITE_VERSION_NUMBER)
>     #endif
>  ] , [res=yes], [res=no])
>  AC_MSG_RESULT([$res])
>  if test "$res" != "yes"; then
>     AC_MSG_ERROR([sqlite version $SQLITE_VERSION or higher required])
>  fi
> 

Could you please attach a patch with this change? It looks very good.

Comment 9

[timeless]

[e-mail]: if it works, please post the patch, looks aren't interesting. tested is better.

Comment 10

[Hussam Al-Tayeb]

Attachment: cls's patch (I only played with the error message)

Thi is cls's patch (I only played with the error message).
It fails with 3.5.6 as expected and works with 3.5.7
Of course maybe cls wants to make further changes.

Comment 11

[Hussam Al-Tayeb]

Can someone take a look at the patch unless cls wanted to make further changes?
I can verify it works for me.

Comment 12

[Shawn Wilsher :sdwilsh]

It needs to have a build peer take a look at it.

Comment 13

[cls]

Comment on attachment 311236 [details] [diff] [review]
cls's patch (I only played with the error message)

There's no need to reparse the SQLLITE_VERSION_NUMBER in the error messasge.  Just use SQLITE_VERSION as that will be defined anyway and the two variables are expected to be in sync.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 311236 [details] [diff] [review]
cls's patch (I only played with the error message)

Also, this never sets SQLITE_CFLAGS or SQLITE_LIBS on success...

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: More like this

Comment 16

[cls]

Comment on attachment 321628 [details] [diff] [review]
More like this

r=cls iff you move the setting of SQLITE_VERSION_NUMBER to the top where SQLITE_VERSION is set.

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

What about comment 3 -- that there was an intent to test for an exact version?

Comment 18

[Mike Hommey [:glandium]]

This was addressed by bug 445164 and bug 551260.