Closed Bug 464088 Opened 16 years ago Closed 16 years ago

Option to build NSS without dbm (handy for WinCE)

Categories

(NSS :: Build, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 5 obsolete files)

As I understand it, we can use sqlite instead which doesn't use the posix io functions that break dbm's build.
Attached patch patch v1 (obsolete) — Splinter Review
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #347347 - Attachment is obsolete: true
Assignee: nobody → doug.turner
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.
until someone fixes dbm for wince, no project will be able to use it.
> until someone fixes dbm for wince,
That work was previously done, long ago.  I'll bet it's still on a branch
somewhere.
nelson, lets just add a configure flag that allows anyone do enable or disable DBM.
Assignee: doug.turner → blassey
Group: mozilla-confidential
Attached patch uses --disable-dbm (obsolete) — Splinter Review
Attachment #347361 - Attachment is obsolete: true
Attachment #349382 - Flags: review?
Attachment #349382 - Flags: review? → review?(nelson)
thanks brad.  this is great.
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.
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
(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?
it should NOT be confidential.
Group: mozilla-confidential
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.
Severity: normal → enhancement
OS: Windows XP → All
Hardware: PC → All
Summary: dbm shoudn't build for wince → Option to build NSS without dbm (handy for WinCE)
Version: unspecified → trunk
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
NSS_DISABLE_DBM looks like it will do nicely for the FTM.
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 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
Attachment #349382 - Flags: review?(nelson) → review-
we can carry the r=ted for the non-nss bits of this patch since they didn't change
Attachment #349382 - Attachment is obsolete: true
Attachment #349795 - Flags: review?(nelson)
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
Attachment #349795 - Flags: review?(nelson) → review-
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.
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.
Attachment #349795 - Attachment is obsolete: true
Attachment #349937 - Flags: review?(nelson)
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.
Attachment #350095 - Flags: review?(rrelyea)
Attachment #349937 - Flags: review?(nelson) → review-
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.
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 on attachment 350095 [details] [diff] [review]
patch - also builds nss/cmd (checked in on trunk)

r+ rrelyea
Attachment #350095 - Flags: review?(rrelyea) → review+
Whiteboard: NSS_NEEDS_CHECKIN
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
Attachment #350095 - Attachment description: patch - also builds nss/cmd → patch - also builds nss/cmd (checked in on trunk)
Priority: -- → P2
Whiteboard: NSS_NEEDS_CHECKIN
Target Milestone: --- → 3.12.3
non-nss patch pushed to m-c

http://hg.mozilla.org/mozilla-central/rev/20a011760de7
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #350111 - Flags: approval1.9.1?
Comment on attachment 350111 [details] [diff] [review]
non-nss parts of previousl patch
[Checked in: Comment 29 & 31]

a191=beltzner
Attachment #350111 - Flags: approval1.9.1? → approval1.9.1+
this landed on the 1.9.1 branch.  adding keyword fixed1.9.1
Keywords: fixed1.9.1
Blocks: 475111
No longer blocks: 475111
Attached patch Supplemental cleanup patch (obsolete) — Splinter Review
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.
Attachment #546457 - Flags: review?(blassey.bugs)
Comment on attachment 546457 [details] [diff] [review]
Supplemental cleanup patch

Review of attachment 546457 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546457 - Flags: review?(blassey.bugs) → review+
Attachment #350111 - Attachment description: non-nss parts of previousl patch → non-nss parts of previousl patch [Checked in: Comment 29 & 31]
Comment on attachment 546457 [details] [diff] [review]
Supplemental cleanup patch

I moved this patch to bug 700066.
Attachment #546457 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.