405290 - Implement version checking for nspr and nss

Status: RESOLVED FIXED

Description

[Mike Hommey [back on Jan 9] [:glandium]]

Currently, nspr.m4 and nss.m4 don't implement version checking. The attached patch adds this.

Comment 1

[Mike Hommey [back on Jan 9] [:glandium]]

Created attachment 290086 [details] [diff] [review]
patch

Comment 2

[Benjamin Smedberg]

Comment on attachment 290086 [details] [diff] [review]
patch

r=me for the changes to build/autoconf

the change in nsprpub/ needs to be a separate patch and reviewed by Wan-Teh, but I'm a little surprised... why would NSPR need a system-NSPR check? I'm betting that file is unused and should be removed.

Comment 3

[Mike Hommey [back on Jan 9] [:glandium]]

(In reply to comment #2)
> (From update of attachment 290086 [details] [diff] [review])
> r=me for the changes to build/autoconf
> 
> the change in nsprpub/ needs to be a separate patch and reviewed by Wan-Teh,
> but I'm a little surprised... why would NSPR need a system-NSPR check? I'm
> betting that file is unused and should be removed.

The answer to that is quite simple: it's not for nspr to use, but for nspr to provide to others. The nspr.m4 file is installed system-wise and available for other configure.ac files to use the AM_PATH_NSPR macro.

Comment 4

[Mike Hommey [back on Jan 9] [:glandium]]

Created attachment 290435 [details] [diff] [review]
Only build/autoconf part

Comment 5

[Mike Hommey [back on Jan 9] [:glandium]]

Created attachment 290438 [details] [diff] [review]
nsprpub part

Comment 6

[Wan-Teh Chang]

Comment on attachment 290438 [details] [diff] [review]
nsprpub part

Thanks for the patch.  The C implementation is here:
http://lxr.mozilla.org/nspr/ident?i=PR_VersionCheck

vminor is equivalent to min_nspr_minor_version.
PR_VMINOR is equivalent to nspr_config_minor_version.
So vminor > PR_VMINOR is equivalent to
"$nspr_config_minor_version" -lt "$min_nspr_minor_version".

I am not familiar with autoconf, but this patch looks correct
except for one bug:

>+		if test "$nspr_config_major_version" -lt "$min_nspr_major_version"; then
>+			no_nspr="yes"

Different major versions of NSPR are incompatible, so -lt
should be changed to -ne.

Comment 7

[Benjamin Smedberg]

Comment on attachment 290435 [details] [diff] [review]
Only build/autoconf part

This needs the same change that wtc indicated below.

Comment 8

[Mike Hommey [back on Jan 9] [:glandium]]

Created attachment 290599 [details] [diff] [review]
new build/autoconf patch

Updated with wtc's comment.

Comment 9

[Mike Hommey [back on Jan 9] [:glandium]]

Created attachment 290600 [details] [diff] [review]
new nsprpub patch

likewise for the nsprpub patch

Comment 10

[Wan-Teh Chang]

Comment on attachment 290600 [details] [diff] [review]
new nsprpub patch

r=wtc.

Comment 11

[Wan-Teh Chang]

Comment on attachment 290600 [details] [diff] [review]
new nsprpub patch

>+		elif test "$nspr_config_major_version" -eq "$min_nspr_major_version" &&
>+		     test "$nspr_config_minor_version" -lt "$min_nspr_minor_version"; then

You can also use just one test command and use -a instead of &&.
(You may need to use a backslash \ to continue the line.)  Not
sure what's the preferred style.

Comment 12

[Mike Hommey [back on Jan 9] [:glandium]]

(In reply to comment #11)
> You can also use just one test command and use -a instead of &&.
> (You may need to use a backslash \ to continue the line.)  Not
> sure what's the preferred style.

-a is not supported on a lot of 'test' implementations.

&& works everywhere. 

Comment 13

[Wan-Teh Chang]

We already use -a in mozilla/configure.in to check for the minimum GNU make version:

http://lxr.mozilla.org/seamonkey/source/configure.in#837

837 if test "$_MAKE_MAJOR_VERSION" -lt "$MAKE_MAJOR_VERSION" || \
838    test "$_MAKE_MAJOR_VERSION" = "$MAKE_MAJOR_VERSION" -a \
839         "$_MAKE_MINOR_VERSION" -lt "$MAKE_MINOR_VERSION"; then
840    AC_MSG_ERROR([GNU Make $MAKE_VERSION or higher is required to build Mozilla.])
841 fi

Thanks for the answer.

Comment 14

[Mike Hommey [back on Jan 9] [:glandium]]

Well, at least for the nsprpub's nspr.m4, it is better to *not* use -a, because it is for use for *external* applications requiring nspr. What is in mozilla's configure.in is mozilla's problem ;)

Comment 15

[Mike Hommey [back on Jan 9] [:glandium]]

Also note the configure.in test you quote is somewhat strange. If -a is supported, so is -o and parenthesis. || is of no use, then.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 290600 [details] [diff] [review]
new nsprpub patch

a=endgame drivers

Comment 17

[Reed Loden [:reed] (use needinfo?)]

Checking in build/autoconf/nspr.m4;
/cvsroot/mozilla/build/autoconf/nspr.m4,v  <--  nspr.m4
new revision: 1.3; previous revision: 1.2
done
Checking in build/autoconf/nss.m4;
/cvsroot/mozilla/build/autoconf/nss.m4,v  <--  nss.m4
new revision: 1.2; previous revision: 1.1
done

wtc, I don't have access to commit the nsprpub patch. Can you do that for Mike, please?

Comment 18

[Wan-Teh Chang]

I checked in the NSPR patch on the NSPR trunk for NSPR 4.7.

Checking in nspr.m4;
/cvsroot/mozilla/nsprpub/config/nspr.m4,v  <--  nspr.m4
new revision: 1.4; previous revision: 1.3
done