263381 - support --with-system-sqlite3 in storage builds

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Need to allow building with a system-installed sqlite3 instead of always
building our own; in particular, Tiger will probably ship with sqlite3.  There's
a few things we need to check for that went in after 3.0.7:

- can create sqlite functions at any point, not just right after a db is opened
(e.g. open, prepare a statement, and then create a function; fails before
3.0.8-ish).
- sqlite3_errcode often was returning a different value than the previous
sqlite3* function returned, fixed at same time as previous bug

Comment 1

[Brett Wilson]

Vlad: I'm pretty sure this is impossible now that we require some command line flags and also support my cache preloading function.

Comment 2

[Mike Hommey [:glandium]]

(In reply to comment #1)
> Vlad: I'm pretty sure this is impossible now that we require some command line
> flags and also support my cache preloading function.

Why don't you externalize these additions to the API, so that you can just use an external sqlite. It's not like it'd be impossible...

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It's never impossible.  Is it a good use of brett's time?  Probably not, given the minimal return we're looking at.

Comment 4

[Mike Hommey [:glandium]]

I'm pretty sure some people at RedHat, Novell, Debian, Ubuntu and others would be interested to spend some time on that issue.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Well, if that's the case, then I look forward to their work; they're certainly welcome to support this, if they're willing to do the work to keep it going.  I'm not very interested in not being able to take a performance or API improvement in the future because some platform's system sqlite3 won't have it.  sqlite's an embeddable database for a reason. :)

Comment 7

[Mike Hommey [:glandium]]

Since bug #306907 has been fixed and apparently mozilla patches have been removed from in-tree sqlite, could this bug be finally considered ?

Comment 8

[Martin Stránský [:stransky] (ni? me)]

I have spent some time with it and I think the only problem is sqlite3file.h - it discloses internal structures which are not exported by regular stand-alone library. 

If we want to link mozilla against external library we have to get rid of that file or export it from the regular stand-alone library...

Comment 9

[Shawn Wilsher :sdwilsh]

We need that file because we use those structures for our async IO.

Comment 10

[Martynas Venckus]

Attachment: Enable native sqlite, v1

Version 2 should also check if sqlite was compiled with SQLITE_ENABLE_REDEF_IO.

Comment 11

[Shawn Wilsher :sdwilsh]

I would only support this if the version of sqlite on the system is the one we are using.  There's been at least one upgrade where sqlite broke functionality that was caught by our unit tests, which makes me very hesitant to do this.  As it stands, this won't even work if you try to link with sqlite 3.5 or greater.

Comment 12

[Martynas Venckus]

Attachment: Enable native sqlite, v2

Version 2 of the first diff.  Adds SQLITE_ENABLE_REDEF_IO and a better version checks.

Comment 13

[Martynas Venckus]

Attachment: Typo in Makefile.in [checked-in]

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 289676 [details] [diff] [review]
Typo in Makefile.in [checked-in]

r=sdwilsh

drivers - this is a no-risk, comment only patch

Comment 15

[Reed Loden [:reed]]

Comment on attachment 289676 [details] [diff] [review]
Typo in Makefile.in [checked-in]

Checking in db/sqlite3/src/Makefile.in;
/cvsroot/mozilla/db/sqlite3/src/Makefile.in,v  <--  Makefile.in
new revision: 1.29; previous revision: 1.28
done

Comment 16

[Mike Hommey [:glandium]]

(In reply to comment #12)
> Created an attachment (id=289675) [details]
> Enable native sqlite, v2
> 
> Version 2 of the first diff.  Adds SQLITE_ENABLE_REDEF_IO and a better version
> checks.
> 

I guess the version check should be changed, and the SQLITE_ENABLE_REDEF_IO check removed, now that sqlite 3.5.4 is in the tree...

Comment 17

[Mike Hommey [:glandium]]

Attachment: Enable native sqlite, against trunk

Not tested, but this should work. I'll ask for review after having tested it.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 303220 [details] [diff] [review]
Enable native sqlite, against trunk

You need to update this file as well, otherwise the check won't be updated.
http://mxr.mozilla.org/seamonkey/source/db/sqlite3/README.MOZILLA

Comment 19

[Mike Hommey [:glandium]]

Attachment: Enable native sqlite

Here you are.

Comment 20

[Mike Hommey [:glandium]]

Attachment: Enable native sqlite

Removed useless AC_SUBST. PKG_CHECK_MODULES does the job (and it's not a problem that it's in a "else" clause, autoconf is m4 and doesn't care about shell code)

Comment 21

[Benjamin Smedberg]

Comment on attachment 303739 [details] [diff] [review]
Enable native sqlite

>--- a/db/sqlite3/src/Makefile.in
>+++ b/db/sqlite3/src/Makefile.in
>@@ -46,6 +46,9 @@ VPATH		= @srcdir@
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE           = sqlite3
>+
>+ifndef MOZ_NATIVE_SQLITE
>+
> LIBRARY_NAME     = sqlite3
> FORCE_SHARED_LIB = 1
> VISIBILITY_FLAGS =
>@@ -96,6 +99,8 @@ DEFINES += -DOS2_HIGH_MEMORY
> endif
> endif
> 
>+endif
>+

Can we put this ifdef in storage/Makefile.in please, instead of here?

Comment 22

[Mike Hommey [:glandium]]

Attachment: Enable native sqlite

Something like this?

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304596 [details] [diff] [review]
Enable native sqlite

a=beltzner for 1.9

Comment 24

[Reed Loden [:reed]]

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1939; previous revision: 1.1938
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 3.450; previous revision: 3.449
done
Checking in config/static-config.mk;
/cvsroot/mozilla/config/static-config.mk,v  <--  static-config.mk
new revision: 3.45; previous revision: 3.44
done
Checking in db/sqlite3/README.MOZILLA;
/cvsroot/mozilla/db/sqlite3/README.MOZILLA,v  <--  README.MOZILLA
new revision: 1.22; previous revision: 1.21
done
Checking in storage/Makefile.in;
/cvsroot/mozilla/storage/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
Checking in storage/build/Makefile.in;
/cvsroot/mozilla/storage/build/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in storage/src/Makefile.in;
/cvsroot/mozilla/storage/src/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/library/libxul-config.mk;
/cvsroot/mozilla/toolkit/library/libxul-config.mk,v  <--  libxul-config.mk
new revision: 1.65; previous revision: 1.64
done

Comment 25

[(not currently active) Ted Mielczarek]

This apparently broke static builds, as the Thunderbird tree is in flames.

Comment 26

[(not currently active) Ted Mielczarek]

Attachment: bustage fix

I checked this in as a bustage fix.  I didn't test it, but it looks right.

Comment 27

[Walter Meinl]

Attachment: remove dist/include/sqlite3 from the includes in system-sqlite builds

Mike, benjamin, reed
grepping through my build log after Mike's patches went in I found
x86_64-pc-linux-gnu-g++ -o mozStorageStatementWrapper.o -c -I../../dist/include/system_wrappers -include ../../config/gcc_hidden.h -DSQLITE_MAX_LIKE_PATTERN_LENGTH=50000 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DGENTOO_LIB_DIR=\"/usr/lib64\"  -I. -I. -I../../dist/include/xpcom -I../../dist/include/string -I../../dist/include/sqlite3 ...
It doesn't hurt since ../dist/include/sqlite3 doesn't exist, however it should probably be cleaned

Comment 28

[Benjamin Smedberg]

Comment on attachment 305195 [details] [diff] [review]
remove dist/include/sqlite3 from the includes in system-sqlite builds

We don't typically ifdef REQUIRES, because it has little value but makes the code harder to read.

Comment 29

[Ginn Chen]

Attachment: fix bustage of Solaris build

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 305683 [details] [diff] [review]
fix bustage of Solaris build

That whole if/else block should be removed and just replaced with EXPAND_LIBNAME_PATH as used elsewhere:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/configure.in&rev=1.1942#910

Note the single quotes so the variables get expanded by make and not the shell.

Comment 31

[Ginn Chen]

Attachment: fix solaris bustage v2

Thanks!

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305998 [details] [diff] [review]
fix solaris bustage v2

a1.9b4=beltzner