835919 - Allow optionally building nss without softoken in the tree

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Elio Maldonado]

The NSS buildsystem should allow downstream nss package maintainers to optionally build nss without softoken (freebl/util) in the source tree via suitable NSS build time environment variables.

Comment 1

[Elio Maldonado]

Fedora and Red Hat Enterprise Linux, and some of their derived distributions, split off the nss build and packaging into three separate but dependent packages. When nss (the higher layers) is built and the tests are run it needs to rely on softoken and util that are already installed on the system. We need to run the tests with the nss on built from the tree and the softoken libraries already installed. That way the testing will reflect what we actually ship to users. The softoken/freebl shared libraries are likely to be of an earlier version that the rest of nss due to it being the last FIPS 140 version.

Comment 2

[Elio Maldonado]

Attachment: Provides option of building nss without softoken sources in the tree

Comment 3

[Elio Maldonado]

Originally, I intended to solve Bug 820207 first and make it a blocker of this one but there are some issues of portability I must resolve there. This one on the other hand, I believe can stand on it's own.

Comment 4

[Robert Relyea]

Comment on attachment 707728 [details] [diff] [review]
Provides option of building nss without softoken sources in the tree

r+ EXCEPT. .../nss/Makefile there should be no need to copy the libraries.

Comment 5

[Elio Maldonado]

(In reply to Robert Relyea from comment #4)

> r+ EXCEPT. .../nss/Makefile there should be no need to copy the libraries.

The problems is that we still have tools left that still link statically with softoken. I have been chasing down tools that either require internal headers or link statically and have cleaned up things a bit not not completely. The ones left are: certcgi, rsaperf, and ocspclnt.

Comment 6

[Robert Relyea]

You can statically link without copying, you just need to include the right -L argument to the static libraries.

Comment 7

[Elio Maldonado]

Hhm, there is not softoken static library installed on the system. The reason I gave, though it has the correct facts, is not the right reason. What the makefile target copies are the shared libraries and the .chk files plus binaries like bltest as had suggested on https://bugzilla.redhat.com/show_bug.cgi?id=689918#c51. Let me review what led me to this and do the experiment in fedora of skipping the copying and see the cause of failures if any at all.

Comment 8

[Robert Relyea]

If the static libraries aren't on the system, then were are you copying them from?:).

bob

Comment 9

[Elio Maldonado]

Attachment: This is the copy script

Copies softoken/freebl shared libraries, their .chk files and a couple of tools.
I'm currently testing a variant that doesn't copy the tools as Bob has suggested. By the way, I'll see if I can inline this into the Makefile and saved me a script.

Comment 10

[Elio Maldonado]

(In reply to Robert Relyea from comment #8)
No static libraries get copied.

Comment 11

[Elio Maldonado]

Did the retesting in fedora and looked at prior versions. The reason for copying the softoken shared libraries is so that the mangling test in test/fips.sh can find the softokn3 shared library to copy into is mangling directory. If it can't find it the test will fail.

Comment 12

[Elio Maldonado]

Comment on attachment 707728 [details] [diff] [review]
Provides option of building nss without softoken sources in the tree

Review of attachment 707728 [details] [diff] [review]:
-----------------------------------------------------------------

r-, from self-review and retesting.

::: ./mozilla/security/nss/lib/manifest.mn
@@ +6,5 @@
>  DEPTH      = ../..
>  
> +UTIL_SRCDIR=util
> +FREEBL_SRCDIR=freebl
> +SOFTOKEN_SRCDIR=softoken

Don't set them no non epmty values first and try to conditionally set them to emty afterwards.

@@ +14,5 @@
> +ifdef USE_SYSTEM_FREEBL
> +ifdef USE_SYSTEM_SOFTOKEN
> +UTIL_SRCDIR=
> +FREEB_SRCDIRL=
> +SOFTOKEN_SRCDIR=

They won't set to empty as we want and the build will ail. I tried it.

@@ +18,5 @@
> +SOFTOKEN_SRCDIR=
> +endif
> +endif
> +endif
> +endif

Do it this simpler way that works

+# Check if we are building nss without softoken
+ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
+UTIL_SRCDIR=
+FREEBL_SRCDIR=
+SOFTOKEN_SRCDIR=
+else
+# default is to compile all
+UTIL_SRCDIR = util
+FREEBL_SRCDIR = freebl
+SOFTOKEN_SRCDIR = softoken
+endif

Comment 13

[Elio Maldonado]

Attachment: Provides option of building nss with softoken in the tree

Comment 14

[Robert Relyea]

> Did the retesting in fedora and looked at prior versions. The reason for copying the softoken
> shared libraries is so that the mangling test in test/fips.sh can find the softokn3 share
> library to copy into is mangling directory. If it can't find it the test will fail.

So update the tests. You have the system build environment variables...

bob

Comment 15

[Elio Maldonado]

Attachment: Allow optionally building nss without softoken, freebl, or util in the tree

Uses the system libraries as Bob suggested.

Comment 16

[Elio Maldonado]

Attachment: Allow optionally building nss without softoken - updated for mercurial

Comment 17

[Robert Relyea]

Comment on attachment 720073 [details] [diff] [review]
Allow optionally building nss without softoken - updated for mercurial

r+ But please make the following changes:

Move your BINDIR setting based on NSS_BUILD_WITHOUT_SOFTOKEN to the tests/common and rename it to FREEBL_BINDIR.

Define an environment variable so the FREEBL_BINDIR can be redefined. (some other platform my put the system installed tests somewhere else.

Comment 18

[Elio Maldonado]

Checked in: https://hg.mozilla.org/projects/nss/rev/351b2d508a32

Comment 19

[Elio Maldonado]

(In reply to Robert Relyea from comment #17)
> Comment on attachment 720073 [details] [diff] [review]
> Allow optionally building nss without softoken - updated for mercurial
> 
> r+ But please make the following changes:
> 
> Move your BINDIR setting based on NSS_BUILD_WITHOUT_SOFTOKEN to the
> tests/common and rename it to FREEBL_BINDIR.
> 
> Define an environment variable so the FREEBL_BINDIR can be redefined. (some
> other platform my put the system installed tests somewhere else.

The same goes for the fips.sh changes at the end where I see the use of /usr/lib{ARCH} to find the system installed version of softokn which is not cross-platform. I'll take advantage of the existing SOFTOKEN_LIB_DIR variable for this.

Comment 20

[Elio Maldonado]

Comment on attachment 720073 [details] [diff] [review]
Allow optionally building nss without softoken - updated for mercurial

Review of attachment 720073 [details] [diff] [review]:
-----------------------------------------------------------------

Fix fips.sh so the softoken shared libray can be found in the system in a portable fashion

::: tests/fips/fips.sh
@@ +225,5 @@
>    mkdir ${MANGLEDIR}
> +
> +  # For nss without softoken builds use the system softoken library
> +  [ "${USE_64}" == "1" ] && export ARCH="64" || export ARCH=""
> +  [ ${NSS_BUILD_WITHOUT_SOFTOKEN = "1" ] && FROM_DIR=/usr/lib${ARCH}/unsupported-tools || FROM_DIR=${BINDIR}

/usr/lib${ARCH} is not cross platform.

@@ +230,2 @@
>    for lib in `ls ${LIBDIR}`; do
> +    if [ ${lib} = ${DLL_PREFIX}softokn3.so ${DESTL} ]; then

${DESTL} is wrong and more impprtantly the test will never succeed bacause when ${NSS_BUILD_WITHOUT_SOFTOKEN = "1" `ls ${LIBDIR}` return a list without softoken int it. I don't see why we loop at all given that the manging is done only on the softoken library. In cause the copying is needed for some other reason I'll leave the loop for now and detect the missing library by testing for its existence in the mangling directory and copying it from the system if not there.

@@ +238,4 @@
>    done
>      
>    echo "$SCRIPTNAME: Detect mangled softoken--------------------------"
>    SOFTOKEN=${MANGLEDIR}/${DLL_PREFIX}softokn3.${DLL_SUFFIX}

check whether it is actually there and if not copy the system one.
if [ ! -e ${SOFTOKEN} ]; then
 # downstrean package maintainer would have set SOFTOKEN_LIB_DIR
 cp ${SOFTOKEN_LIB_DIR}/${DLL_PREFIX}softokn3.${DLL_SUFFIX} ${MANGLEDIR}
fi

Comment 21

[Elio Maldonado]

Attachment: Address Bob's review requests for portability and fix others flaws

Comment 22

[Robert Relyea]

Comment on attachment 742479 [details] [diff] [review]
Address Bob's review requests for portability and fix others flaws

r+ rrelyea

Comment 23

[Elio Maldonado]

Checked in to the tip for nss-3.15
https://hg.mozilla.org/projects/nss/rev/c997ba66a3f2

Comment 24

[Wan-Teh Chang]

Comment on attachment 742479 [details] [diff] [review]
Address Bob's review requests for portability and fix others flaws

Review of attachment 742479 [details] [diff] [review]:
-----------------------------------------------------------------

Elio: I have some comments and questions.

The most important problem is the very first comment about whether
the "USE_SYSTEM_FREEBL" in cmd/platlibs.mk is a typo.

::: cmd/platlibs.mk
@@ +35,5 @@
>  ifdef USE_STATIC_LIBS
>  
>  DEFINES += -DNSS_USE_STATIC_LIBS
>  # $(PROGRAM) has explicit dependencies on $(EXTRA_LIBS)
> +ifndef USE_SYSTEM_FREEBL

1. Is this "USE_SYSTEM_FREEBL" a typo of "NSS_USE_SYSTEM_FREEBL"?

Note that "USE_SYSTEM_FREEBL" also occurs in the comment on line 207.

2. The variable name "NSS_USE_SYSTEM_FREEBL" implies freebl is a separate
package on Fedora, rather than a part of the softoken package. Is that
true?

Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
library, so the variable name is a little misleading.

@@ +41,5 @@
> +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> +else
> +# Use the system freebl and softoken libraries
> +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> +SOFTOKENLIB=

Nit: use spaces around '=' in makefiles. The no-space style is only necessary
in shell scripts.

::: cmd/shlibsign/Makefile
@@ +46,5 @@
>  
>  # sign any and all shared libraries that contain the word freebl
> +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1))
> +CHECKLIBS=
> +CHECKLOC=

Nit: same here: in makefiles, use spaces around '='.

::: lib/Makefile
@@ +65,5 @@
> +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> +# Not included when building nss without softoken
> +UTIL_SRCDIR=
> +FREEBL_SRCDIR=
> +SOFTOKEN_SRCDIR=

Nit: use spaces around '='.

::: lib/manifest.mn
@@ +15,5 @@
>  #  ssl
>  #  smime
>  #  ckfw (builtins module)
>  #  crmf jar (not dll's)
> +DIRS = $(UTIL_SRCDIR) $(FREEBL_SRCDIR) $(SQLITE_SRCDIR) $(DBM_SRCDIR) $(SOFTOKEN_SRCDIR) \

Fold this long line.

::: tests/cipher/cipher.sh
@@ +123,5 @@
>  
>  ################## main #################################################
>  
> +# When building without softoken, bltest isn't built. It was already
> +# built and the cipher suite run as part of an nss-softoken build. 

"cipher suite" => "cipher test suite"?

@@ +125,5 @@
>  
> +# When building without softoken, bltest isn't built. It was already
> +# built and the cipher suite run as part of an nss-softoken build. 
> +if [ ! -x ${DIST}/${OBJDIR}/bin/bltest${PROG_SUFFIX} ]; then
> +    echo "bltest not built, skipping this test." >> ${LOGFILE}

If you skip the cipher.sh tests in this case, why did you make the FREEBL_BINDIR
related changes to allow running the tests with the system installed bltest tool?

::: tests/fips/fips.sh
@@ +233,5 @@
>  
>    echo "mangling ${SOFTOKEN}"
>    echo "mangle -i ${SOFTOKEN} -o -8 -b 5"
> +  # If nss was built without softoken use the system installed one.
> +  # It's location must be specified by the package maintainer.

How does the package maintainer specify the location of the system installed
softoken?  By setting the SOFTOKEN_LIB_DIR environment variable?  The comment
should explain this more precisely.

Comment 25

[Wan-Teh Chang]

Attachment: Remove an extraneous ')' after an ifeq directive in nss/cmd/shlibsign/Makefile

This extraneous ')' causes a warning under GNU make:
    Makefile:48: Extraneous text after `ifeq' directive
but breaks the Mozilla build, whose pymake is stricter than
GNU make.

I also added the spaces around '=' that I suggested.

https://hg.mozilla.org/projects/nss/rev/c02120e4c1e7

Comment 26

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #24)
> Comment on attachment 742479 [details] [diff] [review]
> Address Bob's review requests for portability and fix others flaws
> 
> Review of attachment 742479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Elio: I have some comments and questions.
> 
> The most important problem is the very first comment about whether
> the "USE_SYSTEM_FREEBL" in cmd/platlibs.mk is a typo.
> 
> ::: cmd/platlibs.mk
> @@ +35,5 @@
> >  ifdef USE_STATIC_LIBS
> >  
> >  DEFINES += -DNSS_USE_STATIC_LIBS
> >  # $(PROGRAM) has explicit dependencies on $(EXTRA_LIBS)
> > +ifndef USE_SYSTEM_FREEBL
> 
> 1. Is this "USE_SYSTEM_FREEBL" a typo of "NSS_USE_SYSTEM_FREEBL"?
It should be NSS_USE_SYSTEM_FREEBL. For some reason I exprted USE_SYSTEM_FREEBL=1 in the nss.spec file which is wher I set all the environment variables needed to control the type of build.

> 
> Note that "USE_SYSTEM_FREEBL" also occurs in the comment on line 207.
> 
> 2. The variable name "NSS_USE_SYSTEM_FREEBL" implies freebl is a separate
> package on Fedora, rather than a part of the softoken package. Is that
> true?
> 
> Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
> library, so the variable name is a little misleading.
> 
> @@ +41,5 @@
> > +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> > +else
> > +# Use the system freebl and softoken libraries
> > +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> > +SOFTOKENLIB=

Yes, NSS_USE_SYSTEM_FREEBL is a bit missleading or confusing. I'm actually trying to set both SOFTOKENLIB and CRYPTOLIB. I need to think a bit on how best to change it.
> 
> Nit: use spaces around '=' in makefiles. The no-space style is only necessary
> in shell scripts.
> 
> ::: cmd/shlibsign/Makefile
> @@ +46,5 @@
> >  
> >  # sign any and all shared libraries that contain the word freebl
> > +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1))
> > +CHECKLIBS=
> > +CHECKLOC=
> 
> Nit: same here: in makefiles, use spaces around '='.
> 
> ::: lib/Makefile
> @@ +65,5 @@
> > +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> > +# Not included when building nss without softoken
> > +UTIL_SRCDIR=
> > +FREEBL_SRCDIR=
> > +SOFTOKEN_SRCDIR=
> 
> Nit: use spaces around '='.
> 
> ::: lib/manifest.mn

I'll add spaces for the Makefiles.

> @@ +15,5 @@
> >  #  ssl
> >  #  smime
> >  #  ckfw (builtins module)
> >  #  crmf jar (not dll's)
> > +DIRS = $(UTIL_SRCDIR) $(FREEBL_SRCDIR) $(SQLITE_SRCDIR) $(DBM_SRCDIR) $(SOFTOKEN_SRCDIR) \
> 
> Fold this long line.
Yes, I will change this.

> 
> ::: tests/cipher/cipher.sh
> @@ +123,5 @@
> >  
> >  ################## main #################################################
> >  
> > +# When building without softoken, bltest isn't built. It was already
> > +# built and the cipher suite run as part of an nss-softoken build. 
> 
> "cipher suite" => "cipher test suite"?
That a better name and in a parethethis perhaps say it's the blapi test.

> 
> @@ +125,5 @@
> >  
> > +# When building without softoken, bltest isn't built. It was already
> > +# built and the cipher suite run as part of an nss-softoken build. 
> > +if [ ! -x ${DIST}/${OBJDIR}/bin/bltest${PROG_SUFFIX} ]; then
> > +    echo "bltest not built, skipping this test." >> ${LOGFILE}
> 
> If you skip the cipher.sh tests in this case, why did you make the
> FREEBL_BINDIR
> related changes to allow running the tests with the system installed bltest
> tool?

I think at some point I was trying to rerun the suite but now I don't. I can remove FREEBL_BINDIR. I make need something like this but for experssing the location on the installed un-supported tools but that relates to the other bug for spliiting the rsa performance tool into high and level components whose patch isn't ready yet. Stay tuned.

> 
> ::: tests/fips/fips.sh
> @@ +233,5 @@
> >  
> >    echo "mangling ${SOFTOKEN}"
> >    echo "mangle -i ${SOFTOKEN} -o -8 -b 5"
> > +  # If nss was built without softoken use the system installed one.
> > +  # It's location must be specified by the package maintainer.
> 
> How does the package maintainer specify the location of the system installed
> softoken?  By setting the SOFTOKEN_LIB_DIR environment variable?  The comment
> should explain this more precisely.

I'll point to coreconf/location.mk where three years ago I added varaiables under the control of the package maintainer for finding the location of the sytem installed headers and libraries.  They were modelled on what had been done for NSPR. If the maintainer sets then we use them otherwise we rely on the variables based on the dist directory. I have some commenting to do.

I am considering writing a wiki page. In Fedora we rely on the 
{nss|nss-softokn|nss-util}.spec files as to control the whole building, testing, and packaging process. They could be though at meta Makefiles above the tree. There may be something analogous in other plaforms, say debian-based sytems. Until I do let's see how much clarity I can give with comments in the the Makefiels, manisfest.ms and shell scripts.

Comment 27

[Elio Maldonado]

> Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
> library, so the variable name is a little misleading.
> 
> @@ +41,5 @@
> > +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> > +else
> > +# Use the system freebl and softoken libraries
> > +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> > +SOFTOKENLIB=

The reason that I choose NSS_USE_SYSTEM_FREEBL instead of NSS_USE_SYSTEM_SOFTOKEN is that this is in the use static libraries section. We set CRYPTOLIB to freebl library in the system whereas NSS_USE_SYSTEM_FREEBL is set to empty. A few tools still link statically and so does gblinc on RHEL and Fedora does. On fedora, as well as other Linux distributions, the use of static libraries is frowned upon. An exception was granted for glibc whose little hashing library needs to link statically with libfreebl.a so its gets FIPS 140 compliance via nss. We don't even install a softoken.a static library that's why SOFTOKENLIB is empty. I'll add comments explaining this is as brief a fashion as I can.

Comment 28

[Elio Maldonado]

On NSS_BUILD_WITHOUT_SOFTOKEN we shoudln't build shlibsign either.

Comment 29

[Elio Maldonado]

Attachment: Remove unneeded changes, fix lack o fwhitespace, add better comments and ...

when building nss with softoken skip shlibsign as we do with bltest and fisptest.

Comment 30

[Wan-Teh Chang]

Comment on attachment 745736 [details] [diff] [review]
Remove unneeded changes, fix lack o fwhitespace, add better comments and ...

Review of attachment 745736 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: cmd/Makefile
@@ +16,5 @@
>  
>  ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> +BLTEST_SRCDIR      =
> +FIPSTEST_SRCDIR    =
> +SHLIBSIGN_SRCDIR   =

Nit: in new code, I don't recommend aligning the equal signs like this.
They are a pain to maintain.

::: cmd/platlibs.mk
@@ +41,5 @@
>  SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
>  else
> +# Use the system installed freebl static library and set softoken one to empty.
> +# Some tools need to link statically with freebl but none with softoken. Only
> +# the softoken shared library is installed in the system not the static one.

Nit: put "not the static one" after "shared library".

::: lib/Makefile
@@ +70,4 @@
>  else
>  # default is to include all
> +UTIL_SRCDIR     = util
> +FREEBL_SRCDIR   = freebl

Don't align the equal signs in new code.

All I am asking is that there is one space between a variable and '='.

Comment 31

[Elio Maldonado]

Patched committed to the tip for nss-3.15
https://hg.mozilla.org/projects/nss/rev/6fb9ff40d054