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)
NSS
Build
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)
(deleted),
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
As I understand it, we can use sqlite instead which doesn't use the posix io functions that break dbm's build.
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #347347 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → doug.turner
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
until someone fixes dbm for wince, no project will be able to use it.
Comment 5•16 years ago
|
||
> until someone fixes dbm for wince,
That work was previously done, long ago. I'll bet it's still on a branch
somewhere.
Comment 7•16 years ago
|
||
nelson, lets just add a configure flag that allows anyone do enable or disable DBM.
Assignee: doug.turner → blassey
Group: mozilla-confidential
Assignee | ||
Comment 8•16 years ago
|
||
Attachment #347361 -
Attachment is obsolete: true
Attachment #349382 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #349382 -
Flags: review? → review?(nelson)
Comment 9•16 years ago
|
||
thanks brad. this is great.
Updated•16 years ago
|
Attachment #349382 -
Flags: review+
Comment 10•16 years ago
|
||
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•16 years ago
|
||
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
Assignee | ||
Comment 12•16 years ago
|
||
(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 14•16 years ago
|
||
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.
Updated•16 years ago
|
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
Comment 15•16 years ago
|
||
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•16 years ago
|
||
NSS_DISABLE_DBM looks like it will do nicely for the FTM.
Comment 17•16 years ago
|
||
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•16 years ago
|
||
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-
Assignee | ||
Comment 19•16 years ago
|
||
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)
Comment 20•16 years ago
|
||
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
Updated•16 years ago
|
Attachment #349795 -
Flags: review?(nelson) → review-
Comment 21•16 years ago
|
||
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.
Assignee | ||
Comment 22•16 years ago
|
||
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)
Comment 23•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #349937 -
Flags: review?(nelson) → review-
Comment 24•16 years ago
|
||
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.
Assignee | ||
Comment 25•16 years ago
|
||
Attachment #350111 -
Flags: review+
Assignee | ||
Comment 26•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 350095 [details] [diff] [review]
patch - also builds nss/cmd (checked in on trunk)
r+ rrelyea
Attachment #350095 -
Flags: review?(rrelyea) → review+
Updated•16 years ago
|
Whiteboard: NSS_NEEDS_CHECKIN
Comment 28•16 years ago
|
||
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)
Updated•16 years ago
|
Priority: -- → P2
Whiteboard: NSS_NEEDS_CHECKIN
Target Milestone: --- → 3.12.3
Assignee | ||
Comment 29•16 years ago
|
||
non-nss patch pushed to m-c
http://hg.mozilla.org/mozilla-central/rev/20a011760de7
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #350111 -
Flags: approval1.9.1?
Comment 30•16 years ago
|
||
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+
Assignee | ||
Comment 31•16 years ago
|
||
Comment 32•16 years ago
|
||
this landed on the 1.9.1 branch. adding keyword fixed1.9.1
Keywords: fixed1.9.1
Comment 33•13 years ago
|
||
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)
Assignee | ||
Comment 34•13 years ago
|
||
Comment on attachment 546457 [details] [diff] [review]
Supplemental cleanup patch
Review of attachment 546457 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546457 -
Flags: review?(blassey.bugs) → review+
Updated•13 years ago
|
Attachment #350111 -
Attachment description: non-nss parts of previousl patch → non-nss parts of previousl patch
[Checked in: Comment 29 & 31]
Comment 35•13 years ago
|
||
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.
Description
•