Closed Bug 545036 Opened 11 years ago Closed 11 years ago

Review Gentoo Linux's NSPR package files

Categories

(NSPR :: NSPR, defect)

x86
Linux
defect
Not set
minor

Tracking

(Not tracked)

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: 11 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.