482742 - Enable building util independently of the rest of nss

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Elio Maldonado]

If we wish to built utils as a stand-alone package, independent of nss and softtoken/feebl library then it should have its own version header.

Comment 1

[Elio Maldonado]

One problem we have with building utils stand-alone is that secoid.c needs some pkcs11xx.h headers for the definitions of several CKM_xxx contants it uses. pkcs The pkcs #11 headers are in in softoken where they logically belong. We must prevent circular dependencies yet these headers should not be moved to utils.
A way I was able to checkout sources and build was as follows:
cvs co NSPR
cvs co mozilla/security/coreconf
cvs co -l mozilla/security/nss
cvs co -l mozilla/security/nss/lib
cvs co mozilla/security/nss/lib/util
cvs co -l mozilla/security/nss/lib/softoken/

mv mozilla/security/nss/lib/softoken/pkcs11*.h mozilla/security/nss/lib/util
rm -rf mozilla/security/nss/lib/softoken/

cd mozilla/security/nss
export USE_64=1; gmake nss_build_all

Here I am copying the headers over to utils and deleting the directory which is probably not the cleanest thing to do.

As mentioned in the first comment we also need a version header and a modification of security/coreconf/location.mk. I will attach a patch for that enxt.

Comment 2

[Elio Maldonado]

Attachment: Adds nssutil's include & library directories

Comment 3

[Elio Maldonado]

Attachment: proposed nss util version header

Comment 4

[Wan-Teh Chang]

Elio, perhaps we can solve the header problem by moving
the pkcs11*.h headers from lib/softoken to lib/util.

Kai recently moved some headers that lib/softoken and
lib/freebl need into lib/softoken and lib/freebl for
the same reason.  See these bugs for details:
https://bugzilla.mozilla.org/show_bug.cgi?id=463342
https://bugzilla.mozilla.org/show_bug.cgi?id=463344

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)
> Elio, perhaps we can solve the header problem by moving
> the pkcs11*.h headers from lib/softoken to lib/util.

I won't oppose that approach, if that's our best option.

Comment 6

[Elio Maldonado]

From a logical point of view I would rather have the pkcs #11 headers reside in a pkcs11 headers-only directory rather than in utils. It is for the sake of keeping changes to a minimum that I opt for the current proposal of moving the pkcs11 headers to utils.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 367243 [details] [diff] [review]
Adds nssutil's include & library directories

The right way to do this is not to make coreconf aware of details of building softoken or util, but rather is to change the manifest.mn files for those directories to properly export the header files into the appropriate public and private header file directories.

Comment 8

[Elio Maldonado]

Attachment: add pkcs11 headers to export list on manifest

Comment 9

[Elio Maldonado]

Need to remove them from the softoken manifest.mn, will do in the softoken bug.

Comment 10

[Wan-Teh Chang]

Attachment: Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

Another solution is to add a subset of pkcs11{t,n}.h that
defines the CKM_xxx macros required by secoid.c to lib/util.
This patch illustrates the idea.  It calls the header ckm.h.

Since it's a lot of red tape to add new mechanisms to PKCS
#11, and additional work to implement them in NSS, I don't
expect this header to be updated often.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I'm OK with either 
a) moving all those PKCS11 header files to util, or 
b) Wan-Teh's alternative approach attached above.  

Bob, Julien, any strong views on this?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

As I understand it from today's meeting, there are now three proposals:

1) Original: Move all the PKCS11 header files to util  (bug 483797)

2) Wan-Teh: Duplicate the CKM definitions in a new header in util (comment 10)

3) Bob: Move a subset of the PKCS11 header files to util. 
(I'm not sure I have that right.  I'm not sure what subset is proposed.)  

I think we're all willing to go with whatever Bob decides, but we need to
decide and act quickly.  Bob, please add your comments to this bug.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 367925 [details] [diff] [review]
add pkcs11 headers to export list on manifest

Elio, this patch only does half the job.  The files must be removed from one manifest at the same time that they are added to the other manifest.  So please write a single patch that does both.

Comment 14

[Wan-Teh Chang]

Attachment: Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

Renamed the header mechdefs.h, for "mechanism definitions".
Removed the CKM_xxx macros that secoid.c doesn't need.

It's strange to see that for SHA224 we only need the
CKM_SHA224_HMAC mechanism for secoid.c.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368473 [details] [diff] [review]
Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

I'll ask Bob to review this to be sure he's seen this proposal, and so he will consider this alternative.  

This alternative appeals to me, but Bob expressed dislike of the duplication in today's meeting.

Comment 16

[Wan-Teh Chang]

Comment on attachment 368473 [details] [diff] [review]
Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

I produced this patch to show exactly what secoid.c needs from pkcs11t.h
and pkcs11n.h: 55 CKM_xxx macros.

I removed all comments from pkcs11t.h to make it clear that although
I produced mechdefs.h by copying pkcs11t.h, anyone can easily
reproduce it in a clean-room fashion.  This eliminates the license issue.

I'm wondering if we should replace CKM_SHA224_HMAC and
CKM_SKIPJACK_CBC64 by CKM_INVALID_MECHANISM in the oids table,
since we don't support these two algorithms.

Also, why does the oids table have CKM_xxx_ECB for some but not all
block ciphers?  Is it because no OIDs are defined for some block
ciphers in ECB mode?

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

In reply to Wan-Teh's comment #16:
> I'm wondering if we should replace CKM_SHA224_HMAC and
> CKM_SKIPJACK_CBC64 by CKM_INVALID_MECHANISM in the oids table,
> since we don't support these two algorithms.

I'd say yes, perhaps with a comment that the proper CKMs are those other
ones, but we don't presently support them.  

BTW, should we be adding support for CKM_SHA224_HMAC ?

> Also, why does the oids table have CKM_xxx_ECB for some but not all
> block ciphers?  Is it because no OIDs are defined for some block
> ciphers in ECB mode?

Are you saying that there exist some entries in the oids table with OIDs
defined, but CKM_INVALID_MECHANISM when they should be another mechanism?
Or are you saying that we support mechanisms that are not represented in 
the OIDs table? 

If the latter, it's probably because we've never encountered the OIDs, and
have never learned of any need for them.  Asking for ECB is pretty rare 
anyway.  It's generally avoided due to some vulnerabilities that are 
common to all block ciphers in that mode (as you know), so I'd expect 
demand to be pretty low.

Comment 18

[Robert Relyea]

Comment on attachment 368473 [details] [diff] [review]
Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

r- 

My main issue with this is the pkcs 11 header files are imported from outside. Over the years I have been trying to expunge any NSS specifics in those header files so that we can easily keep in sync. 

This proposal moves away from this goal. It also seems a recipe for errors as it introduces two places where particular values are defined trying to debug a problem if they ever get out of sync would be a night mare.

bob

Comment 19

[Robert Relyea]

I should note that I'm OK with the secoid.c changes, however.


Anyway, Nelson is correct, I'm for either moving all the headers or a subset of those headers. The idea of a mechanism defs.h is really a last resort if the FIPS evaluation forces it on us.

bob

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

OK, so, Elio, Please proceed to write the patch I described in comment 13.
Then we can move forward.

Comment 21

[Wan-Teh Chang]

Comment on attachment 368473 [details] [diff] [review]
Alternative proposal: add a subset of pkcs11{t,n}.h to lib/util

I was about to check in my CKM_NETSCAPE_AES_KEY_WRAP =>
CKM_NSS_AES_KEY_WRAP cleanup change to secoid.c, but then
I found that the whole NSS is still using
CKM_NETSCAPE_AES_KEY_WRAP.  So I aborted my checkin.
It's better to fix all of them at the same time to avoid
confusion.

There is one benefit for moving all the PKCS #11 headers
out of lib/softoken -- if we need to update the headers
for lib/pk11wrap, it won't require a new release of the
softoken package.

Comment 22

[Robert Relyea]

wtc.

I'm OK with you checking in your patch or absorbing it. I don't think we should necessarily wait until we change everything before we make those changes we have in front of us.

bob

Comment 23

[Wan-Teh Chang]

Attachment: Replace CKM_NETSCAPE_AES_KEY_WRAP by CKM_NSS_AES_KEY_WRAP in secoid.c

OK, I checked this in on the NSS trunk (NSS 3.12.3).

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.48; previous revision: 1.47
done

Comment 24

[Elio Maldonado]

Attachment: Move pkcs11 headers from softoken's manifest to util's manifest exports list

In reply to Nelson's request in comment #20. This patch includes all pkcs11 headers. If it's decided to move only a subset (t, n, p, and u) then I'll obsolete this one with a replacement.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368614 [details] [diff] [review]
Move pkcs11 headers from softoken's manifest to util's manifest exports list


Sorry,  here's the problem.  Your "move script" in bug 483797 moves 8 files, which are
pkcs11.h
pkcs11f.h
pkcs11i.h
pkcs11n.h
pkcs11ni.h
pkcs11p.h
pkcs11t.h
pkcs11u.h

But this change to manifest.mn only moves 6 of them.

>+	pkcs11.h \
>+	pkcs11f.h \
>+	pkcs11p.h \
>+	pkcs11t.h \
>+	pkcs11n.h \
>+	pkcs11u.h \

Perhaps the other two belong in "private exports" ??

Comment 26

[Elio Maldonado]

Indeed, we certainly don't want to move those two (i & ni), thanks.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Wouldn't it be better to keep them all together in one directory?
We can use "PRIVATE_EXPORTS" for the *i.h files.

Comment 28

[Wan-Teh Chang]

pkcs11i.h is a softoken-internal header.  It's not even
a private-export header right now.

pkcs11ni.h is a private-export header.  However, its contents
are softoken-specific, so it's better to leave it inside softoken.

Comment 29

[Robert Relyea]

Yes, leave the *i.h in softoken.

bob

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368614 [details] [diff] [review]
Move pkcs11 headers from softoken's manifest to util's manifest exports list

OK, so this patch will be OK, and it's the move script that must change to match it.  

Once the Move script is corrected and reviewed, then here is the sequence of 
events that must take place.

1. The move script is changed into a mozilla.org bug.
2. We wait for the mozilla sysops to "move" (actually, copy) the files. 
   When this is done, the files will appear to be in both places.
3. We commit this patch to the Makefiles.
4. We CVS remove the files from the old location on the trunk only.
(Steps 2 and 3 can be done in one step.) 

It may take a few days for the cvs move to happen, so please get that move
script corrected to day, so I can review it and get the ball rolling.

Comment 31

[Elio Maldonado]

Attachment: Move pkcs11 public headers from softoken manifest to util manifest and add a util version header

Comment 32

[Elio Maldonado]

(In reply to comment #31) This patch is not complete. I need to patch nssutil.rc so it refers to the version numbers for NSSUTIL not NSS.

Comment 33

[Elio Maldonado]

Attachment: Added nssutil.rc modifications to previous patch

By also patching nssutil.rc we break the dependency on nss for its version.

Comment 34

[Elio Maldonado]

Comment on attachment 369199 [details] [diff] [review]
Added nssutil.rc modifications to previous patch

Cancelling, will resend as a smaller patch so I can take care of the pkcs11 headers move portion first.

Comment 35

[Elio Maldonado]

Attachment: Add util version header & update manifest and .rc files

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368614 [details] [diff] [review]
Move pkcs11 headers from softoken's manifest to util's manifest exports list

Previously, I wrote:
> here is the sequence of events that must take place.
>
> 1. The move script is changed into a mozilla.org bug.
> 2. We wait for the mozilla sysops to "move" (actually, copy) the files. 
>    When this is done, the files will appear to be in both places.

we are now at this point, ready for step 3.  Please proceed with the 
steps below.  

> 3. We commit this patch (Attachment 368614 [details] [diff]) to the Makefiles.
> 4. We CVS remove the files from the old location on the trunk only.

Comment 37

[Elio Maldonado]

(In reply to comment #36)  Steps 3 and 4 done.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369304 [details] [diff] [review]
Add util version header & update manifest and .rc files

Moving header files from softoken to util is one thing,
trying to separate the build of util from the rest of NSS,
and give it it's own version number, is quite another.

Some thoughts:

1) Unlike softoken, there's no need or plan to put util on 
a separate release schedule from the rest of NSS, AFAIK,  
so I don't see that it needs a separate header to define a 
separate version number.  I think it should always be BUILT
to be the same version as the rest of NSS.  

2) Unless you're going to pull the source code to build util 
in a separate cvs checkout/update from the rest of NSS 
(which this patch does not do) then util can get by with 
nss's version number header quite nicely.   

3) libNSS and libSoftken will need explicit checks that the 
version number of util is an acceptable version, just as 
libNSS and libSSL now do for softoken, and libNSS does for NSPR.

So, by itself, this patch is incomplete, IMO, but I won't give
it r+ or r- until we work out a complete plan for util.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

I didn't convey what I meant to convey by my first point, so let me add to it.
Separating the built util bits into a separately installable package 
(which is what this bug requests) is NOT the same thing as separating the
source code into a separately pulled and separately built source package.
That said, my points in comment 38 above may make more sense.

Comment 40

[Elio Maldonado]

Actually the goal, if I have understood it correctly, is to be able to post separate source tar archives for fedora/rhel and potentially other downstream platform distributions to make their releases. We will continue posting the full nss source tar ball and additionally something like nss-util, nss-softoken, and nss-rest (probably a better name that this) source tar archives. Most likely any bug fixes to util will wait to be released as part of a broader nss update but having a separate version number for util gives us flexibility. Downstream in Fedora/RHEL we could make separate RPM package with the right dependencies among them.

Comment 41

[Elio Maldonado]

"Package utils separately from the rest of NSS" is not the right title for this bug as this not the intention.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

A way to test this bug is to list a minimal set of directories and files
that must be checked out with CVS into a new source tree workarea in order
to build util, and a script (or make command) that will do that build.

Then, there will also be the matter of making the parts that depend on util
to test to be sure they've loaded with an acceptable version of util.  

So, I think the next step is for you to write that script, or set of steps,
to pull and build util by itself.

Comment 43

[Wan-Teh Chang]

Elio: I think the following are the Fedora/RHEL packages you're
planning to create:

nss-util: libnssutil3.so and the associated headers
nss-softoken: libfreebl3.so, libfreebl3.chk,
              libfreebl.a (for libSSL PKCS #11 bypass mode),
              libsoftokn3.so, libsoftokn3.chk,
              libnssdbm3.so,
              and the associated headers
nss: libnss3.so, libsmime3.so, libssl3.so, libnssckbi.so,
     and the associated headers

The dependency between these packages is:
    nss-softoken depends on nss-util
    nss depends on nss-softoken and nss-util

Am I correct?

Comment 44

[Elio Maldonado]

(In reply to comment #43) 
Yes, you are correct. I actually actually started the investigation downstream in Fedora, with make-believe source tar balls, to discover requirements for this bug and the softoken one. I am now testing building util, softoken, and nss here upstream as Nelson explains in Comment #42.

Comment 45

[Elio Maldonado]

Attachment: adds targets for building utils (also softoken & nss in chained fashion)

Comment 46

[Elio Maldonado]

(In reply to comment #42) 
This are the steps I used to build utils standalone
#!/usr/bin/sh
# requires CVSROOT to be already set
# and also some new targets in the nss Makefile
# Checkout minimal set of directories 
cvs co NSPR
cvs co mozilla/security/coreconf
cvs co -l mozilla/security/nss
cvs co -l mozilla/security/nss/lib
cvs co mozilla/security/nss/lib/util
# build utils
cd mozilla/security/nss; gmake build_util
The makefile target is provided in attachment (id=36950)

Comment 47

[Elio Maldonado]

I mean to say attachment (id=369530)

Comment 48

[Elio Maldonado]

The following proberly belongs in bug 482742 but let me inform here.
For softoken I use
#!/usr/bin/sh
# requires CVSROOT to be already set
# and also some new targets in the nss Makefile
# Checkout minimal set of directories 
cvs co NSPR
# checkouts for building utils
cvs co mozilla/security/coreconf
cvs co -l mozilla/security/nss
cvs co -l mozilla/security/nss/lib
cvs co mozilla/security/nss/lib/util
# the rest for softoken
cvs co mozilla/dbm mozilla/security/dbm
cvs co mozilla/security/nss/lib/freebl
cvs co mozilla/security/nss/lib/softoken
cd mozilla/security/nss; gmake build_softoken
I'm still building utils, instead of linking with a pre-built library

Comment 49

[Elio Maldonado]

In Fedora build system we extract the version numbers (major, minor  and patch) for nss from nss.h.  Modelled after what we do for nss we will like to be able to extract the version numbers out of the proposed nssutil header file as follows:

NSSUTIL_VMAJOR=`cat mozilla/security/nss/lib/util/nssutil.h | grep "#define.*NSSUTIL_VMAJOR" | awk '{print $3}'`

and analogous ones for NSSUTIL_VMINOR and NSSUTIL_VPATCH

Therefore my need for a utils version header.

Comment 50

[Elio Maldonado]

Regarding Comment #46 and #48: The additional targets to the nss Makefile are not absolutely necessary. I can build util and softoken using make nss_build_all just fine. I just get warnings message about non-existent directories but the right stuff gets built.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369304 [details] [diff] [review]
Add util version header & update manifest and .rc files

r+ from me, after you make a few changes to the new .h file.

1) 
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1994-2000
>+ * the Initial Developer. All Rights Reserved.

The above information is incorrect.  Initial developer and copyright dates
are wrong.  

2) Since we're going to RTM on Monday, this should be PR_FALSE, IINM

>+#define NSSUTIL_BETA     PR_TRUE

Comment 52

[Elio Maldonado]

Attachment: header no longer beta and removed incorrect comments as per nelson's review

Comment 53

[Elio Maldonado]

committed to cvs new nssutil.h and changes to manifest.mn and nssutil.c as per revied patch.

Comment 54

[Elio Maldonado]

Attachment: Adds nssutil and softoken include and library directories locations

Comment 55

[Elio Maldonado]

Consuted with Bob and he confirmed that we do need to add these loctions to coreconf/location.mk in order to treat nssutil and softoken as system libraries.

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369783 [details] [diff] [review]
Adds nssutil and softoken include and library directories locations

In reply to comment 55, I think we need a conference call about this.
This seems seriously wrong to me, but maybe you can persuade me with 
much more explanation.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

To elaborate, there is apparently some problem which you've experienced
which is not described here anywhere.  This patch is proposed to resolve it,
but is probably not the only way to resolve it, and I doubt it is the best
way to do so, but I cannot tell without a description of the problem.

Comment 58

[Elio Maldonado]

To clarify, this patch is not needed for the way currently build NSS but is needed because we want the ability to build NSS in a chained fashion. This is analogous to the ability we currently have of separating NSPR from NSS and building them and packaging independently. We currectly do that in Fedora. In the fashion we could chain the build for NSS with.  With '->' standing for "depends on", then NSS -> NSS-{most-of-it} -> NSS-softoken -> NSS-utils.  We could tag and build them individually. Though very unlikely, we could conceivably release a version of utils to fix a critical and urgent bug and not touch the fips-certified softoken or the rest of nss. I hope this helps some.

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369779 [details] [diff] [review]
header no longer beta and removed incorrect comments as per nelson's review

No, you cannot merely remove the section about the initial developer and copyright dates.  That section must be there and must be correct. 
Netscape didn't develop this file, and it wasn't invented before 2009.

Comment 60

[Wan-Teh Chang]

Comment on attachment 369779 [details] [diff] [review]
header no longer beta and removed incorrect comments as per nelson's review

Elio, you can replace this line
    #include "seccomon.h"
by
    #include "prtypes.h"
in nssutil.h because the only thing you need is PR_FALSE/PR_TRUE,
defined in "prtypes.h".

Comment 61

[Elio Maldonado]

Attachment: Remove incorrect attribution to Netscape and include nsprtypes instead of secommon.

This attachment is in response to review comments #59 and #60.

Comment 62

[Elio Maldonado]

Comment on attachment 369822 [details] [diff] [review]
Remove incorrect attribution to Netscape and include nsprtypes instead of secommon.

Misunderstood Comment #59. Will submit a corrected one as part Bug 485713 - Files added by Red Hat recently have missing texts in license headers.

Comment 63

[Elio Maldonado]

Better submit this one here.

Comment 64

[Elio Maldonado]

Attachment: Fixes copyright and includes nsprtypes instead of secommon

Comment 65

[Elio Maldonado]

Comment on attachment 369828 [details] [diff] [review]
Fixes copyright and includes nsprtypes instead of secommon

Sorry, not quite right.

Comment 66

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369783 [details] [diff] [review]
Adds nssutil and softoken include and library directories locations

Elio, this patch needs a lot more explanation.  Saying "Bob says this
is what we need" isn't the right kind of explanation.  

I don't have much problem with the addition of symbols to make 
variables due to the presence of new make or environment variables,
IOW, new actions that are inside of ifdefs, such as these:

>+ifdef NSSUTIL_INCLUDE_DIR
>+    INCLUDES += -I$(NSSUTIL_INCLUDE_DIR)
>+endif

>+ifdef SOFTOKEN_INCLUDE_DIR
>+    INCLUDES += -I$(SOFTOKEN_INCLUDE_DIR)
>+endif

but new actions that are inside ifndef's, such as those shown
below, are another matter because they affect everyone by default.

>+ifndef NSSUTIL_LIB_DIR
>+    NSSUTIL_LIB_DIR = $(DIST)/lib
>+endif

>+ifndef SOFTOKEN_LIB_DIR
>+    SOFTOKEN_LIB_DIR = $(DIST)/lib
>+endif

However, my big issue with those is that they define symbols that
are not used anywhere in NSS (including coreconf, at least according
to mxr), so I'd say this patch is incomplete, at best.  So, there's
no apparent reason to make this change.

Maybe this patch is laying some ground work for another subsequent
patch that will use these symbols.  If so, this patch should be 
reviewed as part of that subsequent larger patch.

Comment 67

[Elio Maldonado]

(In reply to comment #66) Nelson, no problem. You are right, I have realized that this patch is not yet sufficient for it's intended purpose. The purpose being to change the NSS build system just enough to facilitate downstream the chained building of NSS based on its prerequisite components (NSPR, util and soktoken). There are a few aspects of both build systems that I'm trying to understand better.

Comment 68

[Elio Maldonado]

Moving a discussion here that started in Bug 486698. Wan-Teh suggested:

<<Also, the coreconf variables MODULE and REQUIRES can be used to separate nss-util and nss-softoken builds from the main NSS build.  These two variables can be used to ensure the correct dependency between nss-util, nss-softoken, and nss when you build all of them together.  You should consider modifying the manifest.mn files throughout the NSS source tree to set MODULE and REQUIRES properly to reflect the fact that nss-util and nss-softoken are built as separate modules.>>

Comment 69

[Elio Maldonado]

Then in the softoken manifest.mn I would have MODULE=nss-softoken and REQUIRES=dbm nss-util. Moving up to the nss manifest files, they would have MODULE=nss and REQUIRES=nss-softoken. I suppose I don't need to mention nss-util as this is implied. In other words, Is REQUIRES transitive?

Comment 70

[Wan-Teh Chang]

I don't think REQUIRES is transitive.  You should do an experiment
to find out.

Comment 71

[Nelson Bolyard (seldom reads bugmail)]

No, Requires is not transitive.  

If you look in the "dist" directory, e.g.  Mozilla/dist, you will find
two directories (among others) named "public" and "private".  Underneath
those directories, you will find directories by the names of each of the
MODULES named in NSS manifest.mn files.  

A manifest file lists public and/or private header files to be exported, 
by adding the name of those header files to symbols named EXPORTS and/or 
PRIVATE_EXPORTS.  The files named in EXPORTS are exported (copied) into
into the directory .../dist/public/MODULE  (where MODULE is the symbol 
assigned to the make variable MODULE in the manifest.mn file).  The files
named in PRIVATE_EXPORTS are exported to .../dist/private/MODULE.  So,
the MODULE make variable in manifest.mn tells the makefiles the name of 
the directories into which to export the header files.

When the .c files in a directory are built, the compiler command line 
has "-I" options that tell the compiler where to look for include files.
The public and private directories named in MODULE are always used in a 
-I option.  Any additional public directories must be named in the 
REQUIRES make variable in manifest.mn.  Note that there is no way (in 
manifest.mn) to include headers from the private directory of another module.

It is never necessary to list the MODULE name among the REQUIRES names 
because the MODULE name is always implicitly one of the include directories.

For example, if MODULE=nss and REQUIRES=dbm, then the compiler will be 
invoked with 
    -I$(DIST)/public/nss  -I$(DIST)/private/nss -I$(DIST)/public/dbm

Today, we use only two "module" names, which are nss and dbm.  Many 
manifest.mn files list dbm in their "REQUIRES" even though they have no 
dependencies on DBM header files.  While you're hacking on manifest.mn files 
it would be good to fix that.  

We might consider having separate "modules" for each of our groups of 
binaries and/or shared libraries that we want to build independently.
For example, we might have 
- one "module" for util, 
- one for the build unit that includes softoken, freebl, and dbm (a.k.a. 
  the "legacy DB").  
- one for all the other nss shared libraries
- one for NSS commands

I wonder if there are any backward compatibility issues related to putting 
header files into different directories.

Comment 72

[Elio Maldonado]

I have the following modules: 
nss-util     for util
nss-softoken for freebl, and dbm
nss          for the rest of nss (a few tools from nss/cmd had go there)
nss-cmd      for most of the nss commands (save a few)
I had some problems with pk11wrap where some files depend of private structures. Had to move some freebl headers from the private exports list to the public exports list which I don't quite like. I also had to export template.c from utils. Look for these in the upcoming patch.

Comment 73

[Elio Maldonado]

Attachment: changes modules and requires in manifest files, adds nss-cmd for tools

Comment 74

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 72:
> Had to move some freebl headers from the private exports list to
> the public exports list which I don't quite like. I also had to export
> template.c from utils. Look for these in the upcoming patch.

I'm pretty sure we don't want to make any of those files public headers.
It's REALLY REALLY important that we be absolutely clear at all times about
what files are public (meaning, that people outside of the NSS team are 
permitted to call them :).

Maybe we need a new manifest.mn feature, perhaps FRIENDS, that includes
the private header files of another module.

Comment 75

[Elio Maldonado]

I agree with you. Specifically, look for Index: mozilla/security/nss/lib/freebl/manifest.mn in the patch file and you will find algmac.h, blapi.h, and ec.h being moved, something we must avoid. The moving of pk11init.h and pk11parse.h in /mozilla/security/nss/lib/softoken/manifest.mn is another troubling thing.

Comment 76

[Elio Maldonado]

I surely will find more stuff but it's worth sharing what I have so far:
ec.h defines constants that mozilla/NSS/mozilla/security/nss/lib/pk11wrap needs

mozilla/NSS/mozilla/security/nss/lib/ssl/derive.c bypasses the pksc #11 calls freebl directly for functions declared in blapi.h.

Comment 77

[Elio Maldonado]

I bet you know it already but it was new to me. It is ssl3_KeyAndMacDeriveBypass is the sole function that calls blapi functions.

Comment 78

[Elio Maldonado]

The places from where blapi.h is included are the following:
mozilla/security/nss/lib/jar/jarjart.c

mozilla/security/nss/lib/ssl/device.c
mozilla/security/nss/lib/ssl/ssl3con.c
mozilla/security/nss/lib/ssl/ss3ecc.c
mozilla/security/nss/lib/ssl/ssl3ext.c
mozilla/security/nss/lib/ssl/sslsock.c

Also from some of the tools in cmd but I those don't worry me too much as I can change them to be MODULE=nss-softoken thus considering them internal test tools.

Comment 79

[Elio Maldonado]

It seems that it is mostly ssl's need to call directly into freebl what keeps us from doing this module splitting. Any ideas on how to adress this?

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

Accept it as fact.

Comment 81

[Elio Maldonado]

The changes approved so far meet our needs.