6.54 KB, patch
|Details | Diff | Splinter Review|
2.86 KB, patch
|Details | Diff | Splinter Review|
1.97 KB, patch
|Details | Diff | Splinter Review|
3.94 KB, patch
Robert Relyea: review+
|Details | Diff | Splinter Review|
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).
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.
Assignee: nobody → rrelyea
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.
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.
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
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.
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
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.
(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.
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.
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.
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.
Created attachment 410086 [details] [diff] [review] 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.
Assignee: rrelyea → ted.mielczarek
Status: NEW → ASSIGNED
Attachment #410086 - Flags: review?(nelson)
Whiteboard: CONCALL → CONCALL FIPS
Target Milestone: --- → 3.12.5
Version: unspecified → 3.12.4
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.
Attachment #410086 - Flags: review?(nelson) → review?(rrelyea)
Created attachment 410250 [details] [diff] [review] allow specifying SQLITE=whatever on the make commandline Patch against CVS HEAD.
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
Attachment #410250 - Flags: review?(rrelyea) → review+
Since this patch is marked FIPS, it should be checked into the SOFTOKEN_3_13 branch. bob
(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).
Created attachment 410345 [details] [diff] [review] 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.
Attachment #410250 - Attachment is obsolete: true
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: 22.214.171.124; previous revision: 1.29 done
Comment on attachment 410345 [details] [diff] [review] updated patch marking myself as a review reminder.
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.
(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?
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.
(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?
Since bug 513474 doesn't block, neither does this (for reasons outlined in bug 513747 comment 78)
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.
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?
Created attachment 412977 [details] [diff] [review] 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.
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).
I'll try that and see if it does the right thing. Thanks for the suggestion!
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.
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.
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.
What is 10.6 ?
(In reply to comment #34) > What is 10.6 ? Mac OS X Snow Leopard http://en.wikipedia.org/wiki/Mac_OS_X_Snow_Leopard
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.
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.
Created attachment 416763 [details] [diff] [review] 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 on attachment 416763 [details] [diff] [review] Demo patch for NSPR Ted: Is this an acceptable/better approach?
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...)
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)
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.
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?
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.
Landing something in mozilla-central shouldn't affect Firefox 3.6, fwiw.
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 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
Attachment #412977 - Flags: superreview+
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 on attachment 412977 [details] [diff] [review] HG patch updated to tip >+ -$(SQLITE) \ You need to remove '-', otherwise the MSYS build will break.
Created attachment 417036 [details] [diff] [review] 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.
pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/42879234dd4d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
backed out due to Windows orange http://hg.mozilla.org/mozilla-central/rev/99d0d9b3c5d0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
pushed to mozilla-central again, this time with a clobber http://hg.mozilla.org/mozilla-central/rev/f6ee5a7df62f
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago → 8 years ago
Resolution: --- → FIXED
Created attachment 425696 [details] [diff] [review] 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.
Created attachment 442567 [details] [diff] [review] 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 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
Attachment #442567 - Flags: review?(rrelyea) → review+
(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 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: 126.96.36.199; previous revision: 188.8.131.52 done
You need to log in before you can comment on or make changes to this bug.