Closed Bug 288647 Opened 20 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: