Closed Bug 288647 Opened 19 years ago Closed 19 years ago

NSS doesn't build --with-system-nspr

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.8.1

People

(Reporter: wolfiR, Assigned: cls)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 5 obsolete files)

NSS patch v1.1 (checked in to NSS 3.11)
14.95 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
Wan-Teh's patch with typo fixed
1.73 KB, patch
KaiE
: review+
cls
: review+
Details | Diff | Splinter Review 
I found this for Xulrunner but it might be not specific to it.
Maybe it's a pure NSS error.

compiling with --with-system-nspr fails at compiling NSS:

gcc -o Linux2.6_x86_glibc_PTH_OPT.OBJ/quickder.o -c -O2 -fPIC -DLINUX1_2 -Di386
-D_XOPEN_SOURCE -DLINUX2_1 -ansi -Wall -pipe -DLINUX -Dlinux -D_POSIX_SOURCE
-D_BSD_SOURCE -DHAVE_STRERROR -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT
-I/usr/src/packages/BUILD/mozilla/dist/include  -I../../../../dist/public/nss
-I../../../../dist/private/nss -I../../../../dist/include
-I/usr/src/packages/BUILD/mozilla/dist/include/nspr
-I/usr/src/packages/BUILD/mozilla/dist/include/dbm -I../../../../dist/public/dbm
 quickder.c
In file included from quickder.c:43:
secasn1.h:48:21: plarena.h: No such file or directory
--with-system-nspr is also not possible with aviary1.0.1 branch
Attached patch v1.0 (obsolete) — — Splinter Review 
The patch adds NSPR_LIBDIR variable to coreconf and overrides that variable
from security/manager/Makefile.in.  Also fixed the incorrect assumption that
$(DIST) == $(MOZ_BUILD_ROOT)/dist .
Assignee: nobody → cls
Status: NEW → ASSIGNED
Attachment #180407 - Flags: review?(wtchang)
this patch works for i386 but it relies on the NSPR static libs being in
/usr/lib for linking libnssckbi.so.
This is at least not the case for biarch architectures which use /usr/lib64 on
AMD64 systems. I don't know how to solve this within the buildsystem but this
should be fixed somehow.
Perhaps /usr/bin/nspr-config is usable which is provided by NSPR usually.
The value of NSPR_LIBS comes from nspr-config when using --with-system-nspr.  It
sounds like 'nspr-config --libs' isn't returning the correct value on AMD64,
which is a separate problem.

(In reply to comment #4)
> The value of NSPR_LIBS comes from nspr-config when using --with-system-nspr.  It
> sounds like 'nspr-config --libs' isn't returning the correct value on AMD64,
> which is a separate problem.

you're right: bug #289015

caillon: this reminds me that the nspr-devel RPM needs
to install the static libraries libnspr4.a, libplc4.a,
and libplds4.a because NSS needs to link libnssckbi.so
with them.

Although I discourage people from using NSPR static
libraries, I haven't fixed NSS to not do that.

I hope nspr-devel can install the static libraries in
a directory other than /usr/lib.  A directory, like
/usr/lib/nspr4, that is not searched by default for
linking would be good.
Not sure if this has relevance here (the bug was opened against Linux), but FYI,
on Solaris, we only make NSPR shared libraries available with the OS, not static
libraries. The NSPR shared libraries on Solaris are in /usr/lib/mps, or
/usr/lib/mps/64 for the 64-bit versions . I'm not sure if anybody tried to build
with --with-system-nspr on Solaris; my guess is no.

On second thought, I withdraw my suggestion in comment 6.
I am afraid that releasing the NSPR static libraries will
open a can of worms.

I will start working on NSS (libnssckbi.so) so that it
won't link with NSPR static libraries.  But that will be
in the next NSS release, 3.11.  I suggest that we implement
an interim workaround in Mozilla's configure script.  If
--enable-crypto is specified (which is the default on the
Mozilla trunk now), we disallow the --with-system-nspr
--without-system-nss combination.  In other words, if a
build uses system NSPR, it must also use system NSS.
Why would releasing the NSPR static libs open a can of worms?  The default
'real_install' target for NSPR already installs the static libraries. 
Distributors would have to make an effort to *not* install them.

In any case, this means that --with-system-nspr is never going to work because
--with-system-nss doesn't even exist (bug 255408).  Even if it did, I don't
think the two options should be tied together like that.


OK, just for clarification:
- the only thing on earth which needs NSPR static libs is NSS' libnssckbi?
- as we need the NSPR static libs for building NSS or Mozilla or whatever
  mozilla.org project distributors in fact have to ship the static libs
- nobody else should use the static libs and therefore they shouldn't be 
  found from the default linker

If the above assumptions are correct, we have to make sure that NSS or Mozilla
or Firefox can easily be built with dynamic libs of NSPR in default linker
directories while the static libs are hidden somewhere but can be found from the
NSS build process.
(if NSS is standalone or mozilla provided doesn't make any difference in my eyes)
(The not-existant --with-system-nss is another big problem, while it might be
possible to workaround this in build stage)
cls: once you unleash them, it is very hard to take
them back.

My biggest worry about shipping static libraries is
that they are very dependent on compiler versions, and
there are three versions of gcc (3.3, 3.4, and 4.0)
available now.  It is not clear, for example, if gcc 4
compiled objects can be linked with static libraries
compiled with gcc 3.

There are also some features that we only implement
in the shared libraries.  We also compile our code
for use in shared libraries.  (This is especially
the case on Windows, where we use _declspec(dllexport)
on all of our public functions.)

NSS (libnssckbi.so) is the only code in the Mozilla
source tree that must link with NSPR static libraries.

In any case, as long as the static libraries won't
be used by default, and we can ensure that people
need to make a conscious decision to use static
libraries (by forcing them to go through extra hoops),
I am okay with releasing NSPR static libraries in
the nspr-devel RPM.

(NSS static libraries are a different issue.)


one more thing I've observed is that now I get all files in NSPR_LIB
copied to the build-tree and within the archive created by make -C
browser/installer so I end up with a distribution which contains all system libs
from /usr/lib
Wan-Teh:
Since NSPR only uses the C ABI, I don't see why the version of GCC should be an
issue.   Or why it would only be an issue with static libs.  Did the C ABI
change with gcc4 ?

The only issue that I'm aware of with static libs pertains to glibc.  I've seen
glibc throw a warning stating that the same version of glibc must be used at
runtime when using static libs which use functions like dlopen.  But anyone
using static libs should already be aware of this issue.  And I'm not sure if we
use any of those "troublesome" functions in libplds or libplc.  Only the mingw
build links against -lnspr4 and that's dynamically.

If the shared libs are available, the static libs are never used by default. 
The user would have to explicitly link against the file or use -static/-Bstatic
to pickup the static file.

Wolfgang:
I do not see any copies of the NSPR files in my build tree.  Did you start your
build from a clean tree?



(In reply to comment #13)
> Wolfgang:
> I do not see any copies of the NSPR files in my build tree.  Did you start your
> build from a clean tree?

yes. The libs get copied to dist/{firefox,thunderbird} and therefore packaged
into the tar.gz which is created. But not only NSPR libs but also all other libs
which are in the same directory as NSPR libs. They are not in dist/bin or
dist/lib but only in the dist/{firefox,thunderbird} dir.

Ugh. A bad assumption was made in the packaging code. You can set
EXCLUDE_NSPR_LIBS to avoid copying the NSPR & system libs . See
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/Makefile.in#230 .
thanks, but this should be set automatically if --with-system-nspr is
configured, shouldn't it?
It's debatable.  Between system installs of NSPR being uncommon at the time &
sharing that makefile with the Netscape build system, it was easier to let
people set the extra variable if they really didn't want the extra libs.

I let the decision to you. But in newer times there is no Netscape and I (with
my little mind) see no reason to keep this extra option. Maybe there are still
reasons.
Wan-Teh, is the patch OK for you? Would be nice to have this in for 1.8b3.
BTW: This patch is not enough to build standalone NSS against system NSPR, is it?
This is based on the NSS portion of cls's patch v1.0.

It turns out that more changes are needed.  At the
high level, this patch adds two new make variables to
the coreconf build system:

NSPR_INCLUDE_DIR: the directory where NSPR headers reside

NSPR_LIB_DIR: the directory where NSPR libraries reside

If you use a pre-built NSPR (such as "system NSPR"), you
need to set these two make variables when building NSS.

I am not sure where the new coreconf code should be.  I
added them to coreconf/location.mk for lack of an obviously
better place.
Attachment #189377 - Flags: superreview?(nelson)
Attachment #189377 - Flags: review?(rrelyea)
Comment on attachment 180407 [details] [diff] [review]
v1.0

cls, I have some questions about the security/manager/Makefile.in
changes in this patch.

1. Why can't we use $(MOZ_BUILD_ROOT)?

2. We will need to protect the cygpath command now that we also
support MSYS.

3. Is the firstword function defensive programming?  I'd think
there is only one -Lxxx in $(NSPR_LIBS).

Is the strip function also defensive programming?
Comment on attachment 180407 [details] [diff] [review]
v1.0

Looking at mozilla/configure.in, I think that MOZ_BUILD_ROOT
does what ABS_DIST in this patch does, and does it better.
We can do without ABS_DIST and should continue to use
MOZ_BUILD_ROOT.
Comment on attachment 189377 [details] [diff] [review]
NSS patch v1.1 (checked in to NSS 3.11)

I am not so sure about the OS2 and OpenVMS portions of this patch.

Assuming no references ahave been missed (the patch does include cmd). Then
everything else looks ok.

bob
Attachment #189377 - Flags: review?(rrelyea) → review+
I'm not sure about the OS/2 section either. There is no system NSPR on the OS/2
platform, so I don't really get why any change is needed for it.

This bug is filed against the Linux / PC platform only . Is that in error ? If
this bug and the fix are not just for Linux, then the proper data should be in
those fields.

Comment on attachment 189377 [details] [diff] [review]
NSS patch v1.1 (checked in to NSS 3.11)

Bob, thanks for the code review.  The OpenVMS.mk code was
removed because it was the same as the new code added to
location.mk.  The OS2.mk code was removed because
MOZ_COMPONENT_NSPR_LIBS, NSPR_LIBS, and NSPR_INCLUDE_DIR
were unused variables (verified by searching for them in
the "Security" LXR).

Nelson, I'd appreciate if you could at least review the
design decisions described in comment 20.

I've checked in this patch on the NSS trunk (NSS 3.11) for
testing.
Attachment #189377 - Attachment description: NSS patch v1.1 → NSS patch v1.1 (checked in to NSS 3.11)
Comment on attachment 189377 [details] [diff] [review]
NSS patch v1.1 (checked in to NSS 3.11)

To elaborate on the OS/2 changes: the removed code defined three
variables that were not used by NSS/coreconf.  That code may have
been copied from Mozilla's makefiles or configure script.

The code that unset NSPR_INCLUDE_DIR ("NSPR_INCLUDR_DIR =") in
particular must be removed because NSS/coreconf will start to
use that variable.

This patch allows one to build NSS with a pre-built NSPR binary
distribution, which may or may not come with the operating
system.  I've tested this patch on both Linux and Windows.
(Like OS/2, Windows doesn't have "system NSPR", either.)
> 1. Why can't we use $(MOZ_BUILD_ROOT)?

Because we used to allow $(DIST) to set to something other than the default
(currently $(MOZ_BUILD_ROOT)/dist ).  If we don't care about allowing that to be
overridden, then continue to use MOZ_BUILD_ROOT.

> 3. Is the firstword function defensive programming?  I'd think
> there is only one -Lxxx in $(NSPR_LIBS).

There's only one by default.  The nspr-config output can include the -L
directives used in $LDFLAGS when NSPR's configure script is run.

> Is the strip function also defensive programming?

Yes.

I didn't know about this bug until the checkin was done.
This is a wholesale change to the NSS build system, and I 
have heard no prior discussion of this.  
There aren't any sun.com email addresses in the cc list for this bug.

Clearly the model of required environment variables to build NSS has
just been changed.  Would someone here be so kind as to write up a 
statement of the new requirements and send it to mozilla-nssdev-ext?
Nelson: this is a new feature for the NSS/coreconf build system.
If you don't want to use this new feature, nothing changes for
you; you can build NSPR from sources (gmake nss_build_all) or
import (Netscape /share/builds/components style) NSPR binary
distributions (gmake -C ../coreconf; gmake import; gmake -C ../dbm; gmake).

If you want to use this new feature, set NSPR_INCLUDE_DIR to
the directory of NSPR header files and NSPR_LIB_DIR to the
directory of NSPR libraries, and do
    gmake -C ../coreconf; gmake -C ../dbm; gmake
Comment on attachment 189377 [details] [diff] [review]
NSS patch v1.1 (checked in to NSS 3.11)

Looking at the "my requests" page in BMO, I see that a review was requested of
me a week ago.	I never saw any email about it.  Sorry.  I rely on receipt of
requests by email to drive my reviews. 

I surely do wish that BMO would add my email addr to the CC list any time
someone asked me for a patch review.
Attachment #189377 - Flags: superreview?(nelson)
Attachment #180407 - Flags: review?(wtchang)
Attached patch v1.1 (obsolete) — — Splinter Review 
The psm patch has been updated to work with the NSS changes.  It looks like the NSS_CLIENT_TAG still needs to be moved so that the Mozilla build can see the checked in NSS changes.
Attachment #180407 - Attachment is obsolete: true
Attachment #201897 - Flags: review?(wtchang)
Attached patch v1.2 (obsolete) — — Splinter Review 
Check HOST_OS_ARCH == WINNT when using cygpath to avoid cross-compiling bustage.
Attachment #201897 - Attachment is obsolete: true
Attachment #202204 - Flags: review?(wtchang)
Attachment #201897 - Flags: review?(wtchang)
Comment on attachment 202204 [details] [diff] [review]
v1.2

>+ifeq ($(HOST_OS_ARCH),WINNT)
>+ABS_DIST := $(shell cygpath -w $(ABS_DIST) | sed -e 's|\\\\|/|g')
>+endif

Does MSYS have the cygpath command, too?

cygpath has a -m option that uses /.  I know it
is not supported by older versions of Cygwin.  Is
that why you aren't using -m?
Comment on attachment 202204 [details] [diff] [review]
v1.2

No, msys doesn't have the cygpath command.  It auto-translates paths in the shell.  

Yes, we started using cygwin before the -m flag was added to cygpath.  I don't know if it's still practical to use a cygwin installation that is that old but I don't see the point of breaking things if it isn't necessary.
Attachment #202204 - Flags: review?(wtchang)
Attached patch v1.3 (obsolete) — — Splinter Review 
Check for CYGDRIVE_MOUNT before using cygpath.
Attachment #202204 - Attachment is obsolete: true
Attachment #202333 - Flags: review?(wtchang)
Depends on: 317620
Comment on attachment 202333 [details] [diff] [review]
v1.3

cls, thanks a lot for writing this patch and sorry
about the late review.

In comment 27, you said that we used to allow DIST
to be overridden.  Could you elaborate?  In the
current mozilla/config/autoconf.mk.in, I see that
DIST is defined as follows:

 DIST            = $(DEPTH)/dist

So, DIST can't be overridden now.  Since $(MOZ_BUILD_ROOT)
is the absolute pathname version of $(DEPTH), I'd prefer to
just use $(MOZ_BUILD_ROOT)/dist instead of $(ABS_DIST).
If you think it's important to be able to override DIST
in the future, it is fine by me to use ABS_DIST.

I have a question about the following change:

>-DEFAULT_GMAKE_FLAGS += MOZILLA_INCLUDES="-I$(MOZ_BUILD_ROOT)/dist/include/nspr -I$(MOZ_BUILD_ROOT)/dist/include/dbm"
>+DEFAULT_GMAKE_FLAGS += MOZILLA_INCLUDES="$(subst -I$(DIST),-I$(ABS_DIST),$(NSPR_CFLAGS) -I$(DIST)/include/dbm)"

Why don't you use the NSPR_INCLUDE_DIR variable that I added?
You'd set NSPR_INCLUDE_DIR similar to the way you set
NSPR_LIB_DIR, and then MOZILLA_INCLUDES would be just
"-I$(MOZ_BUILD_ROOT)/dist/include/dbm".
Comment on attachment 202333 [details] [diff] [review]
v1.3

I forgot to note that this Mozilla PSM patch depends
on the NSS patch that adds the NSPR_INCLUDE_DIR and
NSPR_LIB_DIR variables.  That NSS patch was only checked
into NSS 3.11.  So this bug is blocked by the landing
of NSS 3.11 on the Mozilla trunk (bug 317620).
Comment on attachment 204079 [details] [diff] [review]
Wan-Teh's patch to illustrate the use of NSPR_INCLUDE_DIR

I didn't test this patch.  I am attaching the patch to
make it concrete my suggestion of using the new
NSPR_INCLUDE_DIR variable.  This patch assumes that we
go with $(ABS_DIST) instead of $(MOZ_BUILD_ROOT)/dist,
partly to simplify the interdiff with patch v1.3.
Attachment #204079 - Attachment description: Wan-Teh' → Wan-Teh's patch to illustrate the use of NSPR_INCLUDE_DIR
Comment on attachment 204079 [details] [diff] [review]
Wan-Teh's patch to illustrate the use of NSPR_INCLUDE_DIR

>+DEFAULT_GMAKE_FLAGS += MOZILLA_INCLUDES=-I$(ABS_DIST)/include/dbm)

This line contains a typo, please remove the final ) at the end of the line.

Otherwise your patch seems to work very well.


I removed all mozilla, nspr, nss rpms from my test system.
I found caillon's nspr.spec file and built a nspr.rpm from NSPR_4_6_1_RC1 and installed it.
I used a MOZILLA_1_8_BRANCH and replaced security/nss, security/coreconf and dbm with NSS_3_11_RC1.
I compiled this version of Mozilla --with-system-nspr
In my dist output directory, no nspr includes were created, and nspr was not compiled in my Mozilla tree.

(However, the resulting binary has a problem with initializing the cert databases, and it runs into an assertion mod!=null at pk11slot.c:1844, I suppose that's a separate problem with NSS 3.11)
Kai,

Thanks a lot for testing the patch.  I knew about the
typo.  I should have attached a new patch.  Sorry.

The problem you ran into is most likely caused by
mozilla/security/manager/Makefile.in not installing the
new libfreebl3.so (and the associated libfreebl3.chk)
of NSS 3.11 in $(DIST)/bin.  We need to update that
makefile for NSS 3.11 as I described in bug 317620.
Attachment #202333 - Attachment is obsolete: true
Attachment #202333 - Flags: review?(wtchang)
This patch is the same as Wan-Teh's, with the type fixed.

It works very good for me.

Wan-Teh was right, my problem was indeed caused by the missing NSS library, everything worked fine after copying the missing library to my dist directory.

r=kaie
Attachment #204079 - Attachment is obsolete: true
Attachment #204174 - Flags: review+
I don't know if you are interested in more thing which only affects NSS build and not NSS within mozilla build:
NSS doesn't have an option to use system NSPR
It almost works now if the env variables NSPR_LIB_DIR and NSPR_INCLUDE_DIR are set.
But make nss_build_all still tries to build an (eventually) not available NSPR via build_nspr target. If the both variables above are set it should skip this IMHO.
Hi Wolfgang, regarding your previous comment, yes, it's necessary to use a different build command when building NSS standalone. If you suggest the NSS build system should detect the presence of the NSPR env variables automatically and build accordingly, you could file an enhancement request bug for the NSS product and Build component and assign it to Wan-Teh.
Attachment #204174 - Flags: review?(cls)
Attachment #204174 - Flags: review?(cls) → review+
I will add this patch to the one in bug 317620, as this one depends on it anyway.
Comment on attachment 204174 [details] [diff] [review]
Wan-Teh's patch with typo fixed

cls, I'm wondering if this patch will break a build tree
whose pathname contains whitespaces.

>+NSPR_LIB_DIR = $(firstword $(filter -L%,$(NSPR_LIBS)))

If the pathname after -L contains whitespaces, does the
above still work?

>+ifneq (,$(strip $(NSPR_LIB_DIR)))
>+NSPR_LIB_DIR := $(subst -L,,$(subst -L$(DIST),-L$(ABS_DIST),$(NSPR_LIB_DIR)))
>+else
>+NSPR_LIB_DIR = $(ABS_DIST)/lib
>+endif

The strip function replaces an internal sequence of one
or more whitespaces with a single space.  So it may also
do bad things to a pathname that contains whitespaces.

>+DEFAULT_GMAKE_FLAGS += MOZILLA_INCLUDES=-I$(ABS_DIST)/include/dbm
>+DEFAULT_GMAKE_FLAGS += SOURCE_MD_DIR=$(ABS_DIST)
>+DEFAULT_GMAKE_FLAGS += DIST=$(ABS_DIST)
>+DEFAULT_GMAKE_FLAGS += NSPR_INCLUDE_DIR=$(NSPR_INCLUDE_DIR)
>+DEFAULT_GMAKE_FLAGS += NSPR_LIB_DIR=$(NSPR_LIB_DIR)

Do we need to quote any of these?
Never mind.  I just did an experiment.  I created the
directory "C:\My Firefox Build" on my Windows machine
and checked out mozilla/client.mk and
mozilla/browser/config/mozconfig in that directory.
I couldn't even use "make -f client.mk checkout MOZ_CO_PROJECT=browser"
to check out the source tree.  So we don't need to worry
about this patch breaking a build tree whose pathname
contains whitespaces, because it's already broken. 
Comment on attachment 204174 [details] [diff] [review]
Wan-Teh's patch with typo fixed

I ran into a problem on Windows.

>+ABS_DIST := $(shell cygpath -w $(ABS_DIST) | sed -e 's|\\\\|/|g')

I had to change "\\\\" to "\\" for this to work.
It seems that in GNU make \ is the escape character
only in patterns (to quote the % characters and other
backslashes).
Wan-Teh, I will attach a new patch to bug 317620 that contains your suggested change.

You also asked me to test this patch, by adding a test make file target in security/manager/Makefile

foo::
	@echo $(NSPR_CFLAGS)
	@echo $(NSPR_LIBS)
	@echo $(DIST)
	@echo $(ABS_DIST)
	@echo $(NSPR_INCLUDE_DIR)
	@echo $(NSPR_LIB_DIR)


I tested it with 2 different object dir builds.

Build 1 uses in-tree nspr and in-tree nss, and the output is:
-I../../dist/include/nspr
-L../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl
../../dist
/home/kaie/moz/head-with-nss311/obj-fire-debug/dist
/home/kaie/moz/head-with-nss311/obj-fire-debug/dist/include/nspr
/home/kaie/moz/head-with-nss311/obj-fire-debug/dist/lib


Build 2 uses system nspr and system nss, and the output is:
-I/usr/include/nspr4
-L/usr/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl
../../dist
/home/kengert/moz/head-with-nss311/obj-fire-debug-sys-nspr-nss/dist
/usr/include/nspr4
/usr/lib

Fix checked in on the Mozilla trunk (Gecko 1.9 alpha) as
part of "Patch v5" (attachment 207994 [details] [diff] [review]) in bug 317620.
Note that the change I described in comment 48 is necessary.

Checking in security/manager/Makefile.in;
/cvsroot/mozilla/security/manager/Makefile.in,v  <--  Makefile.in
new revision: 1.61; previous revision: 1.60
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
I checked in the patch for security/manager/Makefile.in
on the MOZILLA_1_8_BRANCH this afternoon.
Keywords: fixed1.8.1
Target Milestone: mozilla1.9alpha → mozilla1.8.1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: