Closed
Bug 405290
Opened 17 years ago
Closed 17 years ago
Implement version checking for nspr and nss
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta3
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 3 obsolete files)
3.80 KB,
patch
|
benjamin
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
1.92 KB,
patch
|
wtc
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
Currently, nspr.m4 and nss.m4 don't implement version checking. The attached patch adds this.
Assignee | ||
Comment 1•17 years ago
|
||
Comment 2•17 years ago
|
||
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+
Updated•17 years ago
|
Attachment #290086 -
Flags: approval1.9?
Assignee | ||
Comment 3•17 years ago
|
||
(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.
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #290086 -
Attachment is obsolete: true
Attachment #290435 -
Flags: review?
Attachment #290086 -
Flags: approval1.9?
Assignee | ||
Updated•17 years ago
|
Attachment #290435 -
Flags: review? → review?(benjamin)
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #290438 -
Flags: review?(wtc)
Comment 6•17 years ago
|
||
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 7•17 years ago
|
||
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-
Assignee | ||
Comment 8•17 years ago
|
||
Updated with wtc's comment.
Attachment #290435 -
Attachment is obsolete: true
Attachment #290599 -
Flags: review?(benjamin)
Assignee | ||
Comment 9•17 years ago
|
||
likewise for the nsprpub patch
Attachment #290438 -
Attachment is obsolete: true
Attachment #290600 -
Flags: review?(wtc)
Comment 10•17 years ago
|
||
Comment on attachment 290600 [details] [diff] [review] new nsprpub patch r=wtc.
Attachment #290600 -
Flags: review?(wtc) → review+
Comment 11•17 years ago
|
||
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.
Assignee | ||
Comment 12•17 years ago
|
||
(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•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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 ;)
Assignee | ||
Comment 15•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #290599 -
Flags: review?(benjamin) → review+
Updated•17 years ago
|
Attachment #290600 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #290599 -
Flags: approval1.9?
Comment 16•17 years ago
|
||
Comment on attachment 290600 [details] [diff] [review] new nsprpub patch a=endgame drivers
Attachment #290600 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Attachment #290599 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 17•17 years ago
|
||
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
Comment 18•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•