Last Comment Bug 464088 - Option to build NSS without dbm (handy for WinCE)
: Option to build NSS without dbm (handy for WinCE)
Status: RESOLVED FIXED
: fixed1.9.1
Product: NSS
Classification: Components
Component: Build (show other bugs)
: trunk
: All All
: P2 enhancement (vote)
: 3.12.3
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
: 460923 (view as bug list)
Depends on:
Blocks: 694464
  Show dependency treegraph
 
Reported: 2008-11-10 12:53 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2011-11-05 16:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (7.25 KB, patch)
2008-11-10 12:56 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch v2 (12.22 KB, patch)
2008-11-10 13:44 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
uses --disable-dbm (16.95 KB, patch)
2008-11-21 01:13 PST, Brad Lassey [:blassey] (use needinfo?)
nelson: review-
ted: review+
Details | Diff | Splinter Review
removes mcom, removes dead makefile, fixes ruleset.mk, clears DIRS as nelson suggested (26.40 KB, patch)
2008-11-24 11:31 PST, Brad Lassey [:blassey] (use needinfo?)
nelson: review-
Details | Diff | Splinter Review
leaves SDB_LEGACY alone (19.82 KB, patch)
2008-11-25 01:11 PST, Brad Lassey [:blassey] (use needinfo?)
nelson: review-
Details | Diff | Splinter Review
patch - also builds nss/cmd (checked in on trunk) (11.67 KB, patch)
2008-11-25 17:58 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review
non-nss parts of previousl patch [Checked in: Comment 29 & 31] (3.10 KB, patch)
2008-11-25 20:43 PST, Brad Lassey [:blassey] (use needinfo?)
blassey.bugs: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review
Supplemental cleanup patch (1.52 KB, patch)
2011-07-17 20:46 PDT, Wan-Teh Chang
blassey.bugs: review+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2008-11-10 12:53:01 PST
As I understand it, we can use sqlite instead which doesn't use the posix io functions that break dbm's build.
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2008-11-10 12:56:59 PST
Created attachment 347347 [details] [diff] [review]
patch v1
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2008-11-10 13:44:22 PST
Created attachment 347361 [details] [diff] [review]
patch v2
Comment 3 Nelson Bolyard (seldom reads bugmail) 2008-11-11 14:06:27 PST
Comment on attachment 347361 [details] [diff] [review]
patch v2

This patch makes changes to NSS that are desired by the Fennec project,
but it makes them for all users who would build NSS on WinCE. 
In effect, Fennec assumes it is and will be the only use of NSS on WinCE.

I think that if the Fennec project decides to omit parts of NSS from the 
build, the Makefiles should be ifdef FENNEC rather than (or in addition to) 
ifdef WINCE.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2008-11-11 14:09:16 PST
until someone fixes dbm for wince, no project will be able to use it.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2008-11-11 14:41:45 PST
> until someone fixes dbm for wince,
That work was previously done, long ago.  I'll bet it's still on a branch
somewhere.
Comment 6 Doug Turner (:dougt) 2008-11-14 11:51:23 PST
*** Bug 460923 has been marked as a duplicate of this bug. ***
Comment 7 Doug Turner (:dougt) 2008-11-17 09:39:39 PST
nelson, lets just add a configure flag that allows anyone do enable or disable DBM.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2008-11-21 01:13:46 PST
Created attachment 349382 [details] [diff] [review]
uses --disable-dbm
Comment 9 Doug Turner (:dougt) 2008-11-21 14:37:27 PST
thanks brad.  this is great.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2008-11-21 14:50:29 PST
Comment on attachment 349382 [details] [diff] [review]
uses --disable-dbm

+[  --disable-dbm       Disable building dbm],

Should we be more specific and call this --disable-nss-dbm ? I hate having hard-to-decipher build flags.

+ifndef NSS_DISABLE_DBM
 ifdef MOZ_MORK
 DIRS		= mdb mork
 endif
+endif 

Are NSS_DISABLE_DBM and MOZ_MORK mutually exclusive?

--- a/dbm/Makefile.in

I can't for the life of me figure out how the build gets into dbm/. Does NSS call back down into there? Seems silly to comment out the DIRS in this dir, we should just ask NSS not to build in this dir, if possible.

r=me on the mozilla build system bits.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-11-21 15:12:44 PST
I object to NSS bugs being double-secret Confidential Mozilla Project Bugs.
Any change made to NSS needs to have the scrutiny of all NSS developers.
I lack the power to remove that flag, or I would.  

This proposed change simply stops building one of NSS's shared libs.
It doesn't also stop building the code that attempts to use that lib.
It doesn't even change NSS's default so that it defaults to using sql
instead of dbm.  So, the default behavior of NSS on WinCE with this patch 
will be to fail, due to inability to load a required shared lib, unless the
app does one of the 3 methods by which to enable the use of sql.  
I don't think it is acceptable to make NSS fail by default on any platform.

NSS builds dbm in the directory security/dbm.  
The make in that directory is invoked from the Makefile in security/nss.  
Look at the build_dbm target in security/nss/Makefile
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2008-11-21 15:32:21 PST
(In reply to comment #11)
> I object to NSS bugs being double-secret Confidential Mozilla Project Bugs.
> Any change made to NSS needs to have the scrutiny of all NSS developers.
> I lack the power to remove that flag, or I would.  
agreed.  Doug, did you set it for a reason?

> 
> This proposed change simply stops building one of NSS's shared libs.
> It doesn't also stop building the code that attempts to use that lib.
> It doesn't even change NSS's default so that it defaults to using sql
> instead of dbm.  So, the default behavior of NSS on WinCE with this patch 
> will be to fail, due to inability to load a required shared lib, unless the
> app does one of the 3 methods by which to enable the use of sql.  
> I don't think it is acceptable to make NSS fail by default on any platform.

We're not setting the default behavior of windows ce with this patch, instead creating a configure flag to disable dbm building since dbm uses posix io functions not available on windows ce.

The app we're building (xulrunner and anything else built with the mozilla build system) sets the NSS_DEFAULT_DB_TYPE env var to sql.  Is one of the other 2 ways a build flag that we can set so the configure flag can handle it?
Comment 13 Doug Turner (:dougt) 2008-11-21 15:39:09 PST
it should NOT be confidential.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2008-11-21 16:06:36 PST
Thanks for dropping that flag.

I don't object (at all!) to creating a build-time option to build NSS such
that it always uses sql and never uses dbm.  In fact, I welcome it.

Such a project needs to:

a) be configurable on all platforms, enabled through a "feature test macro" 
such as "NSS_SQL_ONLY" or "NSS_NO_DBM" that is not tied to any one platform
(not WINCE). and

b) change the behavior of NSS when built that way so that by default, even without a "sql:" prefix on the DB directory name and without NSS_DEFAULT_DB_TYPE in the environment, NSS will use sql, and not attempt
to use dbm.

I perceive the above two characteristics to be requirements for this RFE.

In addition, there's a LOT more code than just the DBM directory that can 
be left out of such a configuration.  The memory "footprint" of NSS could 
be reduced significantly when built that way.  But I wouldn't require that
for this RFE.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2008-11-21 16:17:13 PST
Changing the default form of DB used by NSS in the absence of a directory
name prefix or NSS_DEFAULT_DB_TYPE envariable is a very simple fix.
I think it's just one line.   This one:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/sftkpars.c&rev=1.6&mark=549#545
Comment 16 Nelson Bolyard (seldom reads bugmail) 2008-11-21 16:19:09 PST
NSS_DISABLE_DBM looks like it will do nicely for the FTM.
Comment 17 Robert Relyea 2008-11-21 16:47:04 PST
This patch conditionally removes mcom_db.h from a number of files in the certdb directory. If everything compliles correctly, I think we should always remove mcom_db.h from those files. I believe the are an ancient historical dreg.

libnssdbm was put in it's own shared library precisely so it could be easy to drop from a distribution (as long as that distribution never needed to reference old NSS dbm databases). It also reduces the in memory footprint of applications that are not actively using the database.

I agree such a distribution should make sql: the default. Nelson has correctly identified the correct line of code to change.
Comment 18 Nelson Bolyard (seldom reads bugmail) 2008-11-21 16:58:07 PST
Comment on attachment 349382 [details] [diff] [review]
uses --disable-dbm

This is close, but a few changes are needed.
There are certain files modified by this patch that I have excluded from 
my review because they are outside of NSS.  Some reviewer for the module(s)
that contain those files should review them.  They are:

mozilla/config/autoconf.mk.in
mozilla/configure.in 
mozilla/db/Makefile.in
mozilla/security/manager/Makefile.in

I believe no change is required to mozilla/dbm/Makefile.in because AFAIK, 
that is a dead file.  The makefiles actually used to build dbm are not 
in this directory but rather are in mozilla/security/dbm. I believe the 
lines you want to undo are:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/dbm/manifest.mn&rev=1.4&mark=47-48#38
and the way to do that is to add some lines in section 6 of 
mozilla/security/dbm/Makefile to conditionally redefine DIRS = $(NULL)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/dbm/Makefile&rev=1.2&mark=73#63
because manifest.mn should have no ifdefs.

Bob's right that, IFF mcom_db.h is not needed, those 8 #includes should just
be deleted.  But If they are needed, then rather than ifdef'ing the 
#include "mcom_db.h" in 8 files, I might suggest putting that ifdef into 
the mcom_db.h source file, bracketing the file's entire contents.  To make 
that work, you would change the patch to the build_dbm target in 
mozilla/security/nss/Makefile to just make the export target, not the libs 
target.  

If mcom_db.h is not needed any more at all, then, rather than changing the 
build_dbm target in mozilla/security/nss/Makefile to do nothing (as this 
patch does), I would suggest changing it to output (echo) a string such as 
"skipping the build of DBM".

Rather than modifying the contents of file
mozilla/security/nss/lib/softoken/legacydb/Makefile to disable the build 
of that directory, you can just add about 3 lines into section 6 of file 
mozilla/security/nss/lib/softoken/Makefile that conditionally redefine 
DIRS = $(NULL)  if the FTM is defined.  
Note: manifest.mn should not have ifdefs.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/Makefile&rev=1.6&mark=73#63
Comment 19 Brad Lassey [:blassey] (use needinfo?) 2008-11-24 11:31:11 PST
Created attachment 349795 [details] [diff] [review]
removes mcom, removes dead makefile, fixes ruleset.mk, clears DIRS as nelson suggested

we can carry the r=ted for the non-nss bits of this patch since they didn't change
Comment 20 Robert Relyea 2008-11-24 14:40:13 PST
Unsolicited comments:

I'm not sure I would removed SDB_LEGACY altogether. It may be preferable to have applications fail when explicitly requesting opening dbm databases rather than silently moving to sql databases.

Your changes a line 548 is sufficient to switch any applications that don't explicitly specify which database to open sql databases and your changes at line 593 are sufficient to prevent selection of dbm databases based on the user setting an environment variable. All other cases someone explicitly decided that the specific database to open was an old dbm database, so I think that case should simply fail (which it will when if fails to load libnssdbm3.so... er.. or nssdbm3.dll).

If you do remove SDB_LEGACY, then you also need to remove SDB_MULTIACCESS. The now defunct multiaccess database code is implemented in nssdbm3 (with appropriate shared library decoration). Multiaccess loaded an external module that had a DBM programming interface, so it used the DBM mapping code.

bob
Comment 21 Nelson Bolyard (seldom reads bugmail) 2008-11-24 19:01:03 PST
Comment on attachment 349795 [details] [diff] [review]
removes mcom, removes dead makefile, fixes ruleset.mk, clears DIRS as nelson suggested

This patch is really close.
I'm delighted to learn that no sources outside of 
nss/lib/softoken/legacydb and dbm itself need mcom_db.h.  Truly.

I agree with Bob 100% that an attempt to use the dbm: or multiaccess: 
prefixes (which explicitly request the use of something like the old DB)
should return an error instead of silently using cert9 and sql instead.  
The change to the default implementation should suffice.
So, do NOT ifdef out SDB_LEGACY.  Thanks.

The proposed change to security/coreconf/ruleset.mk simply breaks the build.
It causes the "for" statement to evaluate $directory as a single value 
which is a string that is the concatenation of all the DIRS, e.g. as
"lib cmd", and of course, you cannot cd to "lib cmd".  So, that change is
bad.  r- for that.
Comment 22 Brad Lassey [:blassey] (use needinfo?) 2008-11-25 01:11:43 PST
Created attachment 349937 [details] [diff] [review]
leaves SDB_LEGACY alone

note that ruleset.mk doesn't handled an empty DIRS out of the box.  If it encounters and empty DIRS, sh can't parse the script (at least on windows) because it evaluates as "for directory in ; do" and ';' is not a valid list.  I've appended "dummy" to the list of dirs to avoid this, which will get ignored after the following test and should be harmless.  If there's a more elegant way to do this, please let me know.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2008-11-25 17:58:58 PST
Created attachment 350095 [details] [diff] [review]
patch - also builds nss/cmd (checked in on trunk)

I was ready to give r+ to the preceding patch, but I applied it to the
trunk and tried to build NSS, and found that the cmd directory did not
build.  So, I fixed that and created a new patch.  Bob, please review.

This patch is basically the same as the previous one, except
a) It only modifies files that are part of NSS,
b) Instead of adding "dummy" to the for command in coreconf/ruleset.mk
it changes the makefiles that are defining DIR = $(NULL) to 
instead define DIR = dummy .  It does not change ruleset.mk .
c) it also modifies security/nss/cmd/platlibs.mk so that the utilities
all build when NSS_DISABLE_DBM is defined.

I have successfully built the CVS trunk of NSS, including libs and cmds, 
both with and without NSS_DISABLE_DBM, with this patch.

I'd like the Fennec guys to test this patch, and confirm here that it 
works for them.  Remember, this patch includes only the NSS files.
Comment 24 Nelson Bolyard (seldom reads bugmail) 2008-11-25 18:13:34 PST
Comment on attachment 349937 [details] [diff] [review]
leaves SDB_LEGACY alone

I almost gave r+ to this patch, but then I tried it.  I pulled the trunk and applied it, and tried building both with and without NSS_DISABLE_DBM.
I found that with that symbol defined, the NSS utility programs don't build.  I have produced a new patch that fixes it.  
It awaits review.
Comment 25 Brad Lassey [:blassey] (use needinfo?) 2008-11-25 20:43:31 PST
Created attachment 350111 [details] [diff] [review]
non-nss parts of previousl patch
[Checked in: Comment 29 & 31]
Comment 26 Brad Lassey [:blassey] (use needinfo?) 2008-11-25 20:45:12 PST
Nelson, you patch built just fine on windows ce.  I've attached the non-nss parts of the previous patch and carried ted's review.
Comment 27 Robert Relyea 2008-11-26 09:18:37 PST
Comment on attachment 350095 [details] [diff] [review]
patch - also builds nss/cmd (checked in on trunk)

r+ rrelyea
Comment 28 Nelson Bolyard (seldom reads bugmail) 2008-12-02 15:29:03 PST
Comment on attachment 350095 [details] [diff] [review]
patch - also builds nss/cmd (checked in on trunk)

Checked in on trunk.

dbm/Makefile.in;                                new: 1.10;  previous: 1.9
security/coreconf/config.mk;                    new: 1.26;  previous: 1.25
security/dbm/Makefile;                          new: 1.3;   previous: 1.2
security/nss/Makefile;                          new: 1.36;  previous: 1.35
security/nss/cmd/platlibs.mk;                   new: 1.59;  previous: 1.58
security/nss/lib/certdb/certdb.c;               new: 1.95;  previous: 1.94
security/nss/lib/certdb/genname.c;              new: 1.37;  previous: 1.36
security/nss/lib/certdb/stanpcertdb.c;          new: 1.83;  previous: 1.82
security/nss/lib/certdb/xauthkid.c;             new: 1.8;   previous: 1.7
security/nss/lib/certdb/xbsconst.c;             new: 1.6;   previous: 1.5
security/nss/lib/certdb/xconst.c;               new: 1.13;  previous: 1.12
security/nss/lib/pk11wrap/secmodi.h;            new: 1.31;  previous: 1.30
security/nss/lib/softoken/Makefile;             new: 1.7;   previous: 1.6
security/nss/lib/softoken/pkcs11.c;             new: 1.155; previous: 1.154
security/nss/lib/softoken/sftkpars.c;           new: 1.7;   previous: 1.6
security/nss/lib/softoken/legacydb/config.mk;   new: 1.8;   previous: 1.7
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2008-12-03 08:43:00 PST
non-nss patch pushed to m-c

http://hg.mozilla.org/mozilla-central/rev/20a011760de7
Comment 30 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-29 14:56:05 PST
Comment on attachment 350111 [details] [diff] [review]
non-nss parts of previousl patch
[Checked in: Comment 29 & 31]

a191=beltzner
Comment 31 Brad Lassey [:blassey] (use needinfo?) 2008-12-31 14:22:59 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/10a15c1e6c57
Comment 32 Doug Turner (:dougt) 2009-01-15 15:14:52 PST
this landed on the 1.9.1 branch.  adding keyword fixed1.9.1
Comment 33 Wan-Teh Chang 2011-07-17 20:46:45 PDT
Created attachment 546457 [details] [diff] [review]
Supplemental cleanup patch

Brad: This patch cleans up your NSS patch (attachment 350095 [details] [diff] [review])
that Nelson checked in for you.

1. In mozilla/security/nss/Makefile, add '@' to the echo command
so that gmake won't echo the echo command.  I also change ifndef
to ifdef and reverse the two branches.

2. mozilla/security/nss/lib/softoken/legacydb/config.mk does not
need to check NSS_DISABLE_DBM because that entire directory is
skipped for NSS_DISABLE_DBM.
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2011-07-18 09:36:59 PDT
Comment on attachment 546457 [details] [diff] [review]
Supplemental cleanup patch

Review of attachment 546457 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 35 Wan-Teh Chang 2011-11-05 16:03:04 PDT
Comment on attachment 546457 [details] [diff] [review]
Supplemental cleanup patch

I moved this patch to bug 700066.

Note You need to log in before you can comment on or make changes to this bug.