Closed Bug 405290 Opened 12 years ago Closed 12 years ago

Implement version checking for nspr and nss

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 3 obsolete files)

Currently, nspr.m4 and nss.m4 don't implement version checking. The attached patch adds this.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Attachment #290086 - Flags: review?(benjamin)
Blocks: 405219
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.
Attachment #290086 - Flags: review?(benjamin) → review+
Attachment #290086 - Flags: approval1.9?
(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.
Attached patch Only build/autoconf part (obsolete) — Splinter Review
Attachment #290086 - Attachment is obsolete: true
Attachment #290435 - Flags: review?
Attachment #290086 - Flags: approval1.9?
Attachment #290435 - Flags: review? → review?(benjamin)
Attached patch nsprpub part (obsolete) — Splinter Review
Attachment #290438 - Flags: review?(wtc)
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.
Attachment #290438 - Flags: review?(wtc) → review-
Comment on attachment 290435 [details] [diff] [review]
Only build/autoconf part

This needs the same change that wtc indicated below.
Attachment #290435 - Flags: review?(benjamin) → review-
Updated with wtc's comment.
Attachment #290435 - Attachment is obsolete: true
Attachment #290599 - Flags: review?(benjamin)
likewise for the nsprpub patch
Attachment #290438 - Attachment is obsolete: true
Attachment #290600 - Flags: review?(wtc)
Comment on attachment 290600 [details] [diff] [review]
new nsprpub patch

r=wtc.
Attachment #290600 - Flags: review?(wtc) → review+
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.
(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. 

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.
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 ;)
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.
Attachment #290599 - Flags: review?(benjamin) → review+
Attachment #290600 - Flags: approval1.9?
Attachment #290599 - Flags: approval1.9?
Comment on attachment 290600 [details] [diff] [review]
new nsprpub patch

a=endgame drivers
Attachment #290600 - Flags: approval1.9? → approval1.9+
Attachment #290599 - Flags: approval1.9? → approval1.9+
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?
Target Milestone: --- → mozilla1.9 M11
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.