519550 - Allow the specification of an alternate library for SQLite

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Shawn Wilsher :sdwilsh]

We'd like to rename sqlite3 to mozsqlite3 to prevent naming issues that are coming up on OS X.

I'm willing to write the patch, but I'll need some help as to where exactly to start.  I'm not great with make, and the NSS build system is different from mozilla-central (which I am more familiar with).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Shawn,
I doubt that renaming is the right answer, but it might be.
If OSX has its own "system" copy of libsqlite3, then it is 
conceivable that an answer is to just use that copy.
Please tell us more about the problem you're trying to solve.

Comment 2

[Shawn Wilsher :sdwilsh]

I'll point you to bug 513747 for more background on this issue.  I don't think using the system copy of SQLite is a good idea for the same reason why we don't (Mozilla) support using system sqlite to build Firefox/Thunderbird: quality assurance.  We test the product with the version of SQLite that we ship with.  Apple will have different versions of SQLite with each release of OS X, and as I understand things, they also modify the SQLite library.  This has the potential to create bugs that you wouldn't see with the controlled copy of SQLite.

Comment 3

[Shawn Wilsher :sdwilsh]

Additionally, we don't always have a system SQLite to link to (like on windows and maemo), but we want to rename the library for all platforms.

Comment 4

[Robert Relyea]

If this is just for Mozilla OS/X, I'm somewhat OK with it.

Pulling that thread could have unexpected consequences, we ran into problems with pcsc-lite on apple. libsqlite3 seems to be a different issue, though. Apple does seem to make a lot of their own changes without running them back up stream, causing some problems. At some point, one will hope that the apple version will stablize and be usable.

bob

Comment 5

[Peter Beckman]

Robert: Really?  It's not OK to change the name of the sqlite library file across the board, but since it's only for OSX, it's somewhat ok?  

While I appreciate that a majority of users are on Windows, or maybe even Linux, I'd hate to think that people feel like a hack here or there or a departure from the normally high quality assurance Mozilla does really isn't important for OSX users.

Apple is inconsistent about the "special things" it does to some standard libraries, and I'd hate to see Firefox break every time Apple does something stupid, when the right decision now could prevent that.  Figuring out how to use the Firefox-packaged libsqlite library seems to be the ideal solution to prevent future pain.

And it seems like renaming the library makes the most sense, to avoid namespace conflicts and to make sure Firefox is stable, both on OSX and across platforms, even where libsqlite doesn't normally exist prior to Firefox installation.  The library is pretty small, and I value the stability that Firefox has had in the past, and would like to encourage that to continue.

Comment 6

[Robert Relyea]

NSS expect to use as sqlite as produced from upstream. Any changes NSS needs, in particular, has already been fed upstream. The people running the SQLite project understand API stability very well. They do not arbitrarily change API semantics on existing API calls.

To the extent someone provides a library called 'sqlite3', they are expected to maintain that stability. If Apple is not abiding by that, I have no problem providing a copy of sqlite3 that does. I do not, however, support punishing other OS's who do maintain faithfulness with the upstream -- particularly on Linux.

I am very much against "we can't trust the OS" mentality until that OS has proven untrustworthy.

So, if Apple is muching with sqlite in the OS, I am in favor of renaming it just for that platform. I am quite strongly opposed to doing so on Linux, however. (Windows is a bit of a wash, as it doesn't have native sqlite support).

bob

Comment 7

[Peter Beckman]

Thanks for the reply Bob.  I understand wanting to respect the locally installed library for Linux, but it leads me to wonder why libsqlite3 was included in the binary distro of Firefox on OSX if by default it should use the local sqlite3 libraries?  

Additionally, when I replaced the Firefox version with the OSX version, I consistently got crashes.  The library seemed to be version 3.6.12, whereas the version included with Firefox seemed to be 3.6.10, but the size of both libraries were very different:

  -rwxr-xr-x  1 beckman  admin  2438496 Oct  8 17:20 libsqlite3.dylib*
  -rwxr-xr-x@ 1 beckman  admin   928744 Sep  3 12:39 libsqlite3.dylib.orig*

And because of the crashes, it seems that the local library cannot be trusted, OR there is something else wrong.

It seems to me that if you are going to package sqlite for Windows, for consistency and testing sake you would want to do the same cross platform.  I agree that it is duplicating code when maybe you don't need to, but when it doesn't work right you have a lot more work to figure out what the problem is, whereas if you know exactly what version of sqlite is tested and distributed against, the chances for instability or crashes is greatly reduced.

If people really want to use THEIR version of sqlite, add an about:config setting to allow for this (if that's possible).  But I think this situation outlines how important sqlite is to Firefox, and how important it is for the sqlite library to be bundled with Firefox cross platform.

Obviously you have to trust the OS to a point, but it's clear to me that the libsqlite3.dylib distributed in FF 3.5.3 is not cleanly compatible with the libsqlite3.dylib that is built into OS 10.6.1.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> To the extent someone provides a library called 'sqlite3', they are expected to
> maintain that stability. If Apple is not abiding by that, I have no problem
> providing a copy of sqlite3 that does. I do not, however, support punishing
> other OS's who do maintain faithfulness with the upstream -- particularly on
> Linux.
I'm not asking to change it for everyone in this bug.  I'm asking for the ability to specify a different one that would be non-default.  Firefox can be built with system sqlite, but the Mozilla Foundation does not support it.  Most (if not all) linux distributions ship their version with it enabled.

The downside, of course, is that most distros don't then run the unit tests with Firefox, and I've seen many bugs come by because they were using a different version of SQLite than what we ship with.

Comment 9

[Peter Beckman]

I see the Bob's point of respecting the OS-installed SQLite instance, but I agree with Shawn that this should be a fix that, by default, uses the SQLite library shipped with Firefox/Mozilla, and allows the user/distro to specify the OS- or user-installed SQLite library if they so choose.  I support using the Firefox-shipped library by default for the stability and testing factor.

Comment 10

[Tim Riley [:timr]]

This bug blocks bug 513747.  Over there Josh says we need his patch plus NSS modifications. Who on the NSS team do we need to reach out to? 

This is crashing firefox when used with a critical tool for web developers and testers, Selenium RC.

Comment 11

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

Yeah, to be clear, we just want the _mechanism_, because otherwise we run into problems on OS X 10.6, and this will let us avoid those problems.

We are not asking that anyone else ship NSS against a different sqlite, etc.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: allow specifying SQLITE=whatever on the make commandline

This is a pretty trivial patch that I believe does what we want. It leaves the default behavior as-is, but allows clients building NSS to override the library name from the make commandline, like "make SQLITE=-lmysqlite3".

I'm not sure who the best reviewer is for NSS build system patches, but I'll target Nelson for it. This patch is against mozilla-central, but I don't believe it will make much difference. If you'd like (and for checkin) I can provide a patch against NSS CVS HEAD.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 410086 [details] [diff] [review]
allow specifying SQLITE=whatever on the make commandline

Ted, please attach a CVS patch against the trunk.
This looks OK to me technically.
The question (potential issue) is whether this change affects the FIPS evaluation.
I'm going to give this review request to Bob to prompt him to address that question.

Bob, Glen, This is a patch to softoken's Makefile.
Is that inside the FIPS boundary?  
Is that one of the files for which changes require approval by the testing labs on behalf of NIST?
If not, please clear the word FIPS from the whiteboard, and complete the review.  Thanks.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: allow specifying SQLITE=whatever on the make commandline

Patch against CVS HEAD.

Comment 15

[Robert Relyea]

Comment on attachment 410250 [details] [diff] [review]
allow specifying SQLITE=whatever on the make commandline

You should also check in nss/cmd as well. There is probably some -lsqlite there.

bob

Comment 16

[Robert Relyea]

Since this patch is marked FIPS, it should be checked into the SOFTOKEN_3_13 branch.

bob

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Since this patch is marked FIPS, it should be checked into the SOFTOKEN_3_13
> branch.
But Nelson only added that because he was not sure if it should be and asked you and/or Glen to indicate if it should or not (see comment 13).

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: updated patch

Thanks, I missed the Windows case in nss/cmd!

I don't believe I have the access to commit this to NSS CVS, so if one of you can make the determination re: FIPS, and land this in the proper place that'd be great. We'd like to get this onto mozilla-central soon.

Comment 19

[Robert Relyea]

I've checked the softoken patch into the SOFTOKEN_3_13 branch.

Glenn has passed a note off to the lab about picking up this change while in process (It's small and very unlikely to have an actual effect on the validation). Things are a bit complicated because we are in the NIST never-land queue waiting period (rumored to be 4-5 months). We're about 2 months in now.

Checking in lib/softoken/config.mk;
/cvsroot/mozilla/security/nss/lib/softoken/config.mk,v  <--  config.mk
new revision: 1.29.4.1; previous revision: 1.29
done

Comment 20

[Robert Relyea]

Comment on attachment 410345 [details] [diff] [review]
updated patch

marking myself as a review reminder.

Comment 21

[(not currently active) Ted Mielczarek]

Thanks, Bob. Please let us know when you get a response.

Could we take a snapshot of the SOFTOKEN_3_13 branch on mozilla-central? I'm not sure what the requirements are for doing that. It would make things a bit less painful for our developers on OS X 10.6, and also let us produce 64-bit OS X builds there.

Comment 22

[Johnathan Nightingale [:johnath]]

(In reply to comment #21)
> Thanks, Bob. Please let us know when you get a response.
> 
> Could we take a snapshot of the SOFTOKEN_3_13 branch on mozilla-central? I'm
> not sure what the requirements are for doing that. It would make things a bit
> less painful for our developers on OS X 10.6, and also let us produce 64-bit OS
> X builds there.

Alternatively, Bob/Nelson - would it make sense for this change to land in 3_12_5? Kai recently filed bug 527659 about getting 3.12.5 onto mozilla-central. Is it also under FIPS-eval-embargo?

Failing that, as Ted asks, is there unpleasantness associated with moving mozilla-central to SOFTOKEN_3_13?

Comment 23

[Robert Relyea]

My preference would be to supply it to you with other changes we expect to have the lab 'sign off' for us now.

We can't really include it in 3.12.5 until we have that sign off, and we can't get the signoff until we get off the NIST 'waiting queue'.  We don't want to do anything that jepordizes our place in that queue.

Talking all of SOFTKEN_3_13 would be the wong choice. We are placing all new changes to softoken in that branch -- including stuff that we don't ask for lab sign off for.

Comment 24

[Johnathan Nightingale [:johnath]]

(In reply to comment #23)
> My preference would be to supply it to you with other changes we expect to have
> the lab 'sign off' for us now.
> 
> We can't really include it in 3.12.5 until we have that sign off, and we can't
> get the signoff until we get off the NIST 'waiting queue'.  We don't want to do
> anything that jepordizes our place in that queue.

Okay, thanks for the clarification, there. I understand you to be saying that, yes, the preferred plan would be to land this on one of the regular 3.12.x (x=4 or x=5 :) branches, but that our next ability to do that can reasonably expected to be late this year/early next.

> Talking all of SOFTKEN_3_13 would be the wong choice. 

Yeah, I figured we probably didn't want to do this, but wanted to be clear.

So - the problem ted and shawn are trying to solve is that using the system SQLite is introducing significant instability for us right now, and while we understand the restraints that FIPS evaluation creates in terms of landing into NSS, we can't really lose our testing/QA abilities for a couple months, either. What are our options? We could patch it locally, but we hate to go off of official NSS tags for reasons I *know* you understand. :)  We could create a one-off branch, but that may or may not make your lives any easier.

Any advice?

Comment 25

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

Since bug 513474 doesn't block, neither does this (for reasons outlined in bug 513747 comment 78)

Comment 26

[Justin Dolske [:Dolske]]

So any followup on the options from comment 24? This is impacting more and more developers, and I don't think waiting months for a solution is an acceptable option. If NSS can't provide a minibranch without NIST restrictions, then we should look at just forking from the NSS release train until the NIST stuff is over with.

And, ftr, having a major piece of the platform tied up in glacially slow NIST process is crazyness. Effectively having our development-hands tied and waiting for even small changes is a mistake that I hope is not repeated in the future.

Comment 27

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

I think NSS needs to provide us with a minibranch on which we can take changes that aren't NIST/FIPS restricted.

WTC, Nelson, Rob: can you chime in, please?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: HG patch updated to tip

Attachment 410345 [details] [diff] doesn't apply cleanly to mozilla-central tip. This patch is updated to tip, but is in hg format.

Comment 29

[Wan-Teh Chang]

Ted: can you add a symlink libsqlite3.so/libsqlite3.dylib
in the dist/lib directory of Mozilla's objdir that points
to libmozsqlite3.so/libmozsqlite3.dylib?  This should allow
the NSS build system to link with -lsqlite3, but use
libmozsqlite3.so/libmozsqlite3.dylib at run time.  This
will avoid the issue of changing a makefile inside the
NSS crypto module (security/nss/lib/softoken/config.mk).

Comment 30

[(not currently active) Ted Mielczarek]

I'll try that and see if it does the right thing. Thanks for the suggestion!

Comment 31

[(not currently active) Ted Mielczarek]

I tried it on OS X, and I don't think it will work. The linker stores the library name as passed on the commandline, so if I run "otool -L libsoftokn3.dylib", I get in the output:
	@executable_path/libsqlite3.dylib (compatibility version 1.0.0, current version 1.0.0)

Which indicates that it's going to try to load that file, and not libmozsqlite3.

Comment 32

[Wan-Teh Chang]

Are you using -install_name @executable_path/libmozsqlite3.dylib
when you build libmozsqlite3.dylib?  You can also try the
-headerpad_max_install_names linker option when you build
libmozsqlite3.dylib.  I remember that linker option allows us
to modify the install name later.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So what is the plan here? There is obviously no progress and in the meantime development slows down for anyone on 10.6.

One alternative is to just take the necessary patches in mozilla-central and then just merge with the nss tree once its working all the platforms that we care about.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

What is 10.6 ?

Comment 35

[(no longer active)]

(In reply to comment #34)
> What is 10.6 ?

Mac OS X Snow Leopard

http://en.wikipedia.org/wiki/Mac_OS_X_Snow_Leopard

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since apparently people don't seem to be understanding the severity here:

Currently firefox does not build and run on all supported platforms. If we had had proper tinderbox coverage, tinderbox would have been permanently orange for the past 3 months (or some such). Usually we close the tree immediately when this happens.

Standing in the way of fixing this is getting this bug fixed. Or rather, reviewing the patch in this bug.

Anyone that uses the latest version of Mac OSX (10.6) can't simply build and run firefox. You have to manually apply the patch in this bug. And any time you want to update your tree you have to back out this patch, update, and then reapply the patch.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, also, if this bug is not fixed by the release of firefox 3.6, this means that anyone that want to build firefox 3.6 on the above mentioned OS, will have to find this bug, download and apply the patch, and can only then get a working build.

Comment 38

[Wan-Teh Chang]

Attachment: Demo patch for NSPR

Re: comment 31

Ted: This patch demonstrates using NSPR that the method I suggested in
comment 29 works on the Mac.

To test it, save this patch as symlink.txt and do the following:
  cvs co NSPR
  patch -p0 < symlink.txt
  cd mozilla/nsprpub
  ./configure
  make
  cd dist/lib
  ls -l

Verify that libnspr4.dylib is a symbolic link pointing to libmoznspr4.dylib.

  otool -L libplc4.dylib

Verify that the output has @executable_path/libmoznspr4.dylib, with "moz"
in the .dylib name.

  cd ../../pr/tests
  make
  otool -L cvar2

Verify the output has @executable_path/libmoznspr4.dylib.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 416763 [details] [diff] [review]
Demo patch for NSPR

Ted: Is this an acceptable/better approach?

Comment 40

[Wan-Teh Chang]

Comment on attachment 416763 [details] [diff] [review]
Demo patch for NSPR

Jonas: this patch is a proof of concept, using NSPR (not NSS).
It is not a fix for this bug.  With Ted's expertise in build
systems, I'm sure he can implement this approach quickly.
(I know how to do it, but I don't have time...)

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ted: can you then comment on if this is an acceptable approach? Or if you prefer to stick to the patch that's already attached (and still awaiting review)

Comment 42

[(not currently active) Ted Mielczarek]

wtc: thanks, I tried your suggestion again, and I think I implemented it incorrectly the first time. It seems to work properly now. I'll update my patch in bug 513747 with this approach, and leave this bug open for getting my other patch (eventually) landed in NSS. I think it's the more correct approach, but we'll stop blocking on it.

Comment 43

[Josh Aas]

Nelson and Bob - you are the NSS module owners which means, I believe, that you can decide what patches we take in the NSS module on mozilla-central. Historically, AFAICT, that has only been merges with official NSS branches. Right now that isn't going to work for us and I think clearly the right thing to do is take the patch here until we can properly get it from an NSS branch merge. If we don't do this we're going to have to take the latest patch on 513747 for now, which is silly (read the patch). I don't want to do something silly when there's really no reason to, we'd be doing it for the sake of a policy that doesn't benefit us here. This temporary divergence from the official branch will not cause any harm while it lasts and it fixes a major immediate problem.

Can you approve landing the patch here as a special case modification to NSS on mozilla-central until we can merge the fix from a branch?

Comment 44

[Wan-Teh Chang]

Josh: based on my understanding of both NSS and Firefox, and the
rules of FIPS validation, I recommend the simple 11-line change to
db/sqlite3/src/Makefile.in in 513747.  It'll avoid a lot of hassles
in verifying that Firefox 3.6 is FIPS compliant.

I will work with Ted to get the NSS patch in this bug reviewed and
checked in, so that you can get rid of the workaround in
db/sqlite3/src/Makefile.in when NSS 3.13 is released.

Comment 45

[Josh Aas]

Landing something in mozilla-central shouldn't affect Firefox 3.6, fwiw.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch in bug 513747 did not work, so we're back to needing the patch in this bug.

Can we get it reviewed quickly? If it only lands on mozilla-central for now then so be it. As I said before, we can always merge with NSS once NSS supports all the platforms we need.

Comment 47

[Robert Relyea]

Comment on attachment 412977 [details] [diff] [review]
HG patch updated to tip

r+ as a temp hack to mozilla central.

Let's keep that working until we can come up with a permanent solution.

The previous patch is already in the SOFTOKEN_3_13_BRANCH. It's also on the priority list to drop into 3_12 once we can get lab sign-offs.

bob

Comment 48

[Wan-Teh Chang]

Comment on attachment 412977 [details] [diff] [review]
HG patch updated to tip

>+SQLITE = $(LIB_PREFIX)sqlite3.$(LIB_SUFFIX)

We should name this variable either SQLITELIB (to follow
the naming convention of CRYPTOLIB, PKIXLIB, and DBMLIB in
this makefile) or SQLITE_LIBS (used in Mozilla build system).

Since this make variable is externally visible, we probably
should use SQLITE_LIBS.  (We already have ZLIB_LIBS.)

Comment 49

[Wan-Teh Chang]

Comment on attachment 412977 [details] [diff] [review]
HG patch updated to tip

>+	-$(SQLITE) \

You need to remove '-', otherwise the MSYS build will break.

Comment 50

[Josh Aas]

Attachment: includes compile fix

This includes the compile fix Wan-Teh mentioned. I don't want to do the naming fix myself right now, I'm more likely to make a mistake with that.

Comment 51

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/42879234dd4d

Comment 52

[Josh Aas]

backed out due to Windows orange

http://hg.mozilla.org/mozilla-central/rev/99d0d9b3c5d0

Comment 53

[Josh Aas]

pushed to mozilla-central again, this time with a clobber

http://hg.mozilla.org/mozilla-central/rev/f6ee5a7df62f

Comment 55

[Kai Engert (:KaiE:)]

Attachment: patch merged for nss 3.12.6

FYI, in bug 544450 it has been proposed to rename variable SQLITE to SQLITE_LIB_NAME. I haven't done so yet, I don't know whether this would cause additional complication for the FIPS review process.

When landing NSS 3.12.6 (beta) to mozilla-central, I had to merge the patch because of a minor context change, I'm attaching the latest patch here.

Comment 56

[Wan-Teh Chang]

Attachment: Proposed patch (use SQLITE_LIB_NAME) (checked in)

I propose that we use this patch instead of the current patch we're
using for Firefox ("patch merged for nss 3.12.6", attachment 425696 [details] [diff] [review]).

Firefox will need to build NSS with
    SQLITE_LIB_NAME=mozsqlite3
instead of
    SQLITE=$(call EXPAND_LIBNAME,mozsqlite3)
See http://mxr.mozilla.org/mozilla-central/search?string=mozsqlite

Comment 57

[Robert Relyea]

Comment on attachment 442567 [details] [diff] [review]
Proposed patch (use SQLITE_LIB_NAME) (checked in)

r+ pending one issue...

in config.mk for softoken,

I believe the SQLITE_LIB_NAME should bo after the -L NSSUTIL_LIB_DIR. In the case you are building on a platform which doesn't have sqlite and are using the one provided by NSS, that library will come from the same directory as the NSSUTIL_LIB_DIR. Some platforms and combinations may break if you don't move it.

Mozilla builds and builds that use system sqlite (like Linux rpm's) will not fail.

bob

Comment 58

[Wan-Teh Chang]

(In reply to comment #57)
>
> in config.mk for softoken,
> 
> I believe the SQLITE_LIB_NAME should bo after the -L NSSUTIL_LIB_DIR.

This is not a problem, because we have:

 EXTRA_SHARED_LIBS += \
 	-L$(DIST)/lib \
+	-l$(SQLITE_LIB_NAME) \
 	-L$(NSSUTIL_LIB_DIR) \
 	-lnssutil3 \
-	-lsqlite3 \

So -L$(DIST)/lib will cover -l$(SQLITE_LIB_NAME) if we are using the sqlite
provided by NSS.

Comment 59

[Wan-Teh Chang]

Comment on attachment 442567 [details] [diff] [review]
Proposed patch (use SQLITE_LIB_NAME) (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.7).

Checking in coreconf/location.mk;
/cvsroot/mozilla/security/coreconf/location.mk,v  <--  location.mk
new revision: 1.13; previous revision: 1.12
done
Checking in nss/cmd/platlibs.mk;
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.67; previous revision: 1.66
done
Checking in nss/lib/softoken/config.mk;
/cvsroot/mozilla/security/nss/lib/softoken/config.mk,v  <--  config.mk
new revision: 1.30; previous revision: 1.29
done

And on the SOFTOKEN_3_13_BRANCH (Softoken 3.13).

Checking in config.mk;
/cvsroot/mozilla/security/nss/lib/softoken/config.mk,v  <--  config.mk
new revision: 1.29.4.2; previous revision: 1.29.4.1
done