Closed Bug 545036 Opened 15 years ago Closed 15 years ago

Review Gentoo Linux's NSPR package files

Categories

(NSPR :: NSPR, defect)

Product:
NSPR
Component:
NSPR
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

I'd like to review how Gentoo Linux builds and installs its NSPR package. I do this review as a voluntary service. Gentoo Linux's NSPR package files can be found at http://sources.gentoo.org/viewcvs.py/gentoo-x86/dev-libs/nspr/
Comments on nspr-4.8.3.ebuild, rev. 1.1: 1. Remove DEPEND=">=dev-db/sqlite-3.5" because NSPR does not depend on sqlite. (NSS does.) 2. In src_compile(), remove the following configure options: * --enable-system-sqlite: I don't think this configure option exists. * --with-mozilla: It may not be obvious, but this option is not necessary. If you are not convinced, you can leave it for now. * --with-pthreads: It's not necessary to specify --with-pthreads because using pthreads is the default. It should be harmless to specify --with-pthreads, but I would delete the option. 3. In src_compile(), I don't know what the following two lines mean: $(use_enable ipv6) \ $(use_enable debug) \ I just wanted to make sure you do NOT specify --enable-ipv6. That option is not necessary for Linux. On Linux NSPR will automatically detect at run time if IPv6 can be used. 4. You should specify the following configure options --disable-debug --enable-optimize which are equivalent to specifying BUILD_OPT=1 for NSS, because NSPR's build system does a debug build by default. 5. I suggest that you not create the .so.${MINOR_VERSION} files for NSPR's shared libraries. It is a historic mistake that NSPR uses nonstandard SONAME. NSPR puts the major version (4) in the shared library's name instead of after the .so. In other words, the file name libnspr4.so should have been libnspr.so.4 The wrong position of 4 is a mere cosmetic issue, so I recommend that you not change the SONAME. If you really have to change it, you can see that it should be .so.${MAJOR_VERSION} instead of .so.$(MINOR_VERSION}. 6. I suggest that you install the NSPR shared libraries in /usr/lib instead of /usr/lib/nspr.
(In reply to comment #1) > Comments on nspr-4.8.3.ebuild, rev. 1.1: > > 1. Remove > DEPEND=">=dev-db/sqlite-3.5" > because NSPR does not depend on sqlite. (NSS does.) easily fixable. > > 2. In src_compile(), remove the following configure options: > * --enable-system-sqlite: I don't think this configure option > exists. > * --with-mozilla: It may not be obvious, but this option is > not necessary. If you are not convinced, you can leave it > for now. > * --with-pthreads: It's not necessary to specify --with-pthreads > because using pthreads is the default. It should be harmless > to specify --with-pthreads, but I would delete the option. > These can also be removed without a problem. > 3. In src_compile(), I don't know what the following two lines mean: > $(use_enable ipv6) \ > $(use_enable debug) \ > use_enable is just that, if the user has the useflag enabled it would pass --enable-debug, and if useflag is disable --disable-debug for example. > I just wanted to make sure you do NOT specify --enable-ipv6. That > option is not necessary for Linux. On Linux NSPR will automatically > detect at run time if IPv6 can be used. > > 4. You should specify the following configure options > --disable-debug --enable-optimize > which are equivalent to specifying BUILD_OPT=1 for NSS, because > NSPR's build system does a debug build by default. I can make that change. > 5. I suggest that you not create the .so.${MINOR_VERSION} > files for NSPR's shared libraries. It is a historic mistake > that NSPR uses nonstandard SONAME. NSPR puts the major > version (4) in the shared library's name instead of after > the .so. In other words, the file name > libnspr4.so > should have been > libnspr.so.4 > The wrong position of 4 is a mere cosmetic issue, so I > recommend that you not change the SONAME. If you really > have to change it, you can see that it should be > .so.${MAJOR_VERSION} instead of .so.$(MINOR_VERSION}. This was changes back to upstream as porting patch for each release was becoming more hassle then it was worth, Maybe an upstream bug should be opened and get this fixed once and for all. > 6. I suggest that you install the NSPR shared libraries > in /usr/lib instead of /usr/lib/nspr. This will not happen due to our file protection, we ensure we do not clobber same file from another package.
Re: use_enable I am not sure if --disable-ipv6 works. I hope you can remove $(use_enable ipv6) As for $(use_enable debug) NSPR only supports these two combinations of configure options properly: --enable-debug --disable-optimize # this is the default --disable-debug --enable-optimize So please make sure your use of --enable-debug/--disable-debug is linked to a corresponding --disable-optimize/--enable-optimize. Re: /usr/lib/nspr Just to clarify: is this a Gentoo Linux policy, or are you worried that some other package may also have a file named libnspr4.so, libplc4.so, or libplds4.so? Does Gentoo install the shared libraries of package "foo" in /usr/lib/foo? I'm just curious... Thanks.
Status: NEW → ASSIGNED
Wan-Teh, I appreciate all your knowledge, I have commited an update for our testing branch. It still installs to /usr/$(get_libdir/nspr, soon as soname issue is fixed upstream we will move it down to /usr/$(get_libdir), I believe all other changes have been addressed, once again thanks for your help to clean up our build. Jory --- nspr-4.8.3.ebuild 2010-02-07 13:13:59.000000000 -0600 +++ nspr-4.8.3-r1.ebuild 2010-02-08 22:13:56.751322766 -0600 @@ -13,10 +13,7 @@ LICENSE="|| ( MPL-1.1 GPL-2 LGPL-2.1 )" SLOT="0" KEYWORDS="~alpha ~amd64 ~arm ~hppa ~ia64 ~mips ~ppc ~ppc64 ~sparc ~x86 ~x86-fbsd" -IUSE="ipv6 debug" - -DEPEND=">=dev-db/sqlite-3.5" -RDEPEND="${DEPEND}" +IUSE="debug" src_unpack() { unpack ${A} @@ -44,12 +41,11 @@ *) die "Failed to detect whether your arch is 64bits or 32bits, disable distcc if you're using it, please";; esac - myconf="${myconf} --libdir=/usr/$(get_libdir)/nspr \ - --enable-system-sqlite --with-mozilla --with-pthreads" + myconf="${myconf} --libdir=/usr/$(get_libdir)/nspr" ECONF_SOURCE="../mozilla/nsprpub" econf \ - $(use_enable ipv6) \ $(use_enable debug) \ + $(use_enable !debug optimize) \ ${myconf} || die "econf failed" make CC="$(tc-getCC)" CXX="$(tc-getCXX)" || die } I would like ya but will take about an hour before our cvs source is avaliable for view.
Err that should be link you. But to also answer your question no, we only install out of norm if package has a problem with another package, for instance nss installs libssl.a while openssl also installs libssl.a hense they need to be installed to seperate directories.
(In reply to comment #4) Jory, I don't understand the language used to write .ebuild files, but your patch in comment 4 looks good to me. Thanks!
(In reply to comment #5) Thanks for the answer. In that case, you should still install NSPR's three shared libraries (libnspr4.so, libplc4.so, and libplds4.so) in /usr/lib, because they don't conflict with any other package. Again, do not install the static libraries (libnspr4.a, libplc4.a, and libplds4.a). Does NSPR's "make install" target do that? If so, I should fix that.
I reviewed all the other files. Among the .ebuild files, I only reviewed nspr-4.8.3-r1.ebuild rev. 1.1. They all look good to me. The only patch that might be good to contribute to NSPR upstream is nspr-4.7.0-prtime.patch. In summary, the remaining work is: 1. Do not install the static libraries (libnspr4.a, libplc4.a, and libplds4.a). 2. Install the shared libraries in /usr/lib instead of /usr/lib/nspr. 3. Consider contributing nspr-4.7.0-prtime.patch to upstream.
(In reply to comment #8) > In summary, the remaining work is: > 1. Do not install the static libraries (libnspr4.a, libplc4.a, > and libplds4.a). This will need to be fixed upstream, the static libs are installed via make install > 2. Install the shared libraries in /usr/lib instead of > /usr/lib/nspr. Once upstream removes static libs, I will more nspr to use /usr/lib > 3. Consider contributing nspr-4.7.0-prtime.patch to > upstream. I can open a bug to attempt to get this included upstream.
Jory: Thanks. I filed bug 545449 on the "make install" issue. We can declare this review done now. Does Gentoo's policy prohibit installing static libraries in /usr/lib? libnspr4.a, libplc4.a, and libplds4.a are unlikely to conflict with another package.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(In reply to comment #10) > Jory: Thanks. I filed bug 545449 on the "make install" issue. > We can declare this review done now. > > Does Gentoo's policy prohibit installing static libraries in /usr/lib? > libnspr4.a, libplc4.a, and libplds4.a are unlikely to conflict with > another package. No the policy does not, I could make the move right now but I see no point really. I would rather not force my users to rebuild for a simp]e lib move.
(In reply to comment #10) > Jory: Thanks. I filed bug 545449 on the "make install" issue. > We can declare this review done now. > > Does Gentoo's policy prohibit installing static libraries in /usr/lib? > libnspr4.a, libplc4.a, and libplds4.a are unlikely to conflict with > another package. Wan-Teh, I want to thank you again for all your advise. I have moved nspr to /usr/lib{64} accordingly, and removed all static libraries. The only issue that has not been addressed is the soname, I still believe we should work to get this fixed upstream so everyone benefits without having to patch each release.
(In reply to comment #1) > Comments on nspr-4.8.3.ebuild, rev. 1.1: > > * --with-pthreads: It's not necessary to specify --with-pthreads > because using pthreads is the default. It should be harmless > to specify --with-pthreads, but I would delete the option. > I just wanted to make sure you do NOT specify --enable-ipv6. That > option is not necessary for Linux. On Linux NSPR will automatically > detect at run time if IPv6 can be used. According to Gentoo guidelines, "Packages should not configure and link based upon what is available at compile time — any autodetection must be overridden." http://devmanual.gentoo.org/general-concepts/use-flags/index.html So it's wrong to have the package detect the presence of pthreads and ipv6 (or anything else for that matter.) This should be controlled by USE flags.
Thanks for the info on Gentoo's guidelines. The issue is that NSPR's build system has some build options that are obsolete or not applicable to Linux. We documented the options that should be used by everyone in https://developer.mozilla.org/en/NSPR_build_instructions. The other options should be used only if you understand the relevant parts of the NSPR build system well. Official NSPR releases for Linux are built *without* specifying --with-pthreads and --enable-ipv6. It is redundant to specify --with-pthreads on Linux. If you specify --with-pthreads, it's harmless. It may break NSPR if you specify --enable-ipv6 on the wrong platform. Do not specify --enable-ipv6 on Linux. It is best to consider --with-pthreads and --enable-ipv6 as build options not applicable to Linux.
You need to log in before you can comment on or make changes to this bug.