Last Comment Bug 567620 - Bump minimum required version for system NSPR to 4.8.6
: Bump minimum required version for system NSPR to 4.8.6
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: ---
Assigned To: Micah Gersten
:
Mentors:
Depends on:
Blocks: 560582
  Show dependency treegraph
 
Reported: 2010-05-22 22:36 PDT by Micah Gersten
Modified: 2010-10-01 10:17 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.11-fixed
.14-fixed


Attachments
Patch to bump system NSPR version requirement (400 bytes, patch)
2010-05-22 22:36 PDT, Micah Gersten
benjamin: review-
Details | Diff | Splinter Review
Bump to 4.8.6 (397 bytes, patch)
2010-08-06 08:55 PDT, Micah Gersten
ted: review+
christian: approval1.9.2.11+
christian: approval1.9.1.14+
Details | Diff | Splinter Review
Add prlog.h to fix the build issue with NSPR less than 4.8.6 (738 bytes, patch)
2010-09-06 04:26 PDT, Micah Gersten
wtc: review+
christian: approval1.9.2.11-
Details | Diff | Splinter Review

Description Micah Gersten 2010-05-22 22:36:33 PDT
Created attachment 446942 [details] [diff] [review]
Patch to bump system NSPR version requirement

This is preventing trunk builds with system NSPR.  This is due the fix for bug 560582 which requires a function not in 4.8.4. I suggest bumping the minimum NSPR to 4.8.4.99
Comment 1 Benjamin Smedberg [:bsmedberg] 2010-05-24 13:16:47 PDT
Comment on attachment 446942 [details] [diff] [review]
Patch to bump system NSPR version requirement

Where did .99 come from? Is that an actual version number? AFAIK there isn't a current released version of NSPR that works with trunk.
Comment 2 Micah Gersten 2010-05-24 13:20:07 PDT
(In reply to comment #1)
> (From update of attachment 446942 [details] [diff] [review])
> Where did .99 come from? Is that an actual version number? AFAIK there isn't a
> current released version of NSPR that works with trunk.

Right, so the last time something like this happened, it was suggested to bump to *.99 since there was no official release yet.  If there is another preferred method, I'd be happy to adjust the patch.
Comment 3 Benjamin Smedberg [:bsmedberg] 2010-06-04 07:43:24 PDT
This blocks final, but I don't really want to take the intermediate patch, just leave the bug until there is a real NSPR version.
Comment 4 Micah Gersten 2010-06-04 08:16:25 PDT
(In reply to comment #3)
> This blocks final, but I don't really want to take the intermediate patch, just
> leave the bug until there is a real NSPR version.

No problem, I fixed our build system locally so that it does the check, so I don't actually need this ATM.  Thanks.
Comment 5 Micah Gersten 2010-08-06 08:55:10 PDT
Created attachment 463562 [details] [diff] [review]
Bump to 4.8.6

Bump to 4.8.6 after landing of bug 575620
Comment 6 Micah Gersten 2010-08-13 09:32:48 PDT
Comment on attachment 463562 [details] [diff] [review]
Bump to 4.8.6

bug 576447 landed on mozilla-1.9.2 and mozilla-1.9.1, so this is needed as well
Comment 7 christian 2010-08-16 14:19:14 PDT
Comment on attachment 463562 [details] [diff] [review]
Bump to 4.8.6

a=LegNeato for 1.9.2.9 and 1.9.1.12.

This needs to be landed as soon as possible on mozilla-1.9.2 default and mozilla-1.9.1 default.
Comment 8 christian 2010-08-16 19:08:10 PDT
Actually, I need to understand this a bit more, so setting approvals back to "?".

I don't see why this is needed. This is what I understand:

* Clearly, when we link with our own NSPR we always have the correct version so we won't hit this
* Bug 575620 caused us to update to a newer NSPR on 1.9.2 only, which should still not require us to bump the min version if we don't use any new functionality
* Bug 576447 landed on both branches and uses PR_STATIC_ASSERT, but PR_STATIC_ASSERT exists in 1.9.1 and is used all over 1.9.2 going back to 3.6. I don't see how that landing could cause us to need a newer version on 1.9.2 but not on 1.9.1 (where the new version didn't even land).

So, as far as I can tell we shouldn't need to bump the min version for the branches. Am I missing something?

If it's not needed I'd rather not alter the min version.
Comment 9 Micah Gersten 2010-08-16 20:37:59 PDT
(In reply to comment #8)
> Actually, I need to understand this a bit more, so setting approvals back to
> "?".
> 
> I don't see why this is needed. This is what I understand:
> 
> * Clearly, when we link with our own NSPR we always have the correct version so
> we won't hit this
> * Bug 575620 caused us to update to a newer NSPR on 1.9.2 only, which should
> still not require us to bump the min version if we don't use any new
> functionality
> * Bug 576447 landed on both branches and uses PR_STATIC_ASSERT, but
> PR_STATIC_ASSERT exists in 1.9.1 and is used all over 1.9.2 going back to 3.6.
> I don't see how that landing could cause us to need a newer version on 1.9.2
> but not on 1.9.1 (where the new version didn't even land).
> 
> So, as far as I can tell we shouldn't need to bump the min version for the
> branches. Am I missing something?
> 
> If it's not needed I'd rather not alter the min version.

I got a build error on 191 and 192 after bug 576447 actually landed on the branches using system NSPR 4.8.4.  I can get you the build logs if you want.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-19 05:08:16 PDT
http://hg.mozilla.org/mozilla-central/rev/4cdc47a945a7
Comment 11 christian 2010-08-19 10:25:59 PDT
Micah, a build log would be great so we can see if it is needed on the branches.
Comment 13 christian 2010-08-23 14:38:06 PDT
Adding in come build system peers to figure out if we really need this on branches. Again, my understanding from comment 8 doesn't seem to reflect what is actually happening.
Comment 14 Wan-Teh Chang 2010-08-23 15:36:30 PDT
It seems that you just need to change nsHTMLFrameSetElement.cpp
to include the NSPR header that defines PR_STATIC_ASSERT.

PR_STATIC_ASSERT was added in NSPR 4.7 (see bug 375985).

In NSPR 4.7 - 4.8.4, include "prlog.h".

In NSPR 4.8.6 or later, include "prtypes.h". (Since every
NSPR header includes "prtypes.h", this is satisfied if
you include any NSPR header.)

Note: there is no NSPR 4.8.5 release.
Comment 15 christian 2010-08-25 10:43:14 PDT
(In reply to comment #14)
> It seems that you just need to change nsHTMLFrameSetElement.cpp
> to include the NSPR header that defines PR_STATIC_ASSERT.
> 
> PR_STATIC_ASSERT was added in NSPR 4.7 (see bug 375985).
> 
> In NSPR 4.7 - 4.8.4, include "prlog.h".

Which is how 1.9.1 is (http://mxr.mozilla.org/mozilla1.9.1/source/content/html/content/src/nsHTMLFrameSetElement.cpp#46).

1.9.1 should be using nspr < 4.8.6, as we didn't take bug 575620 on that branch. I don't see why 1.9.1 would suddenly be broken after the fix for bug 576447, as that file already includes prlog.h which defines PR_STATIC_ASSERT.

> 
> In NSPR 4.8.6 or later, include "prtypes.h". (Since every
> NSPR header includes "prtypes.h", this is satisfied if
> you include any NSPR header.)

Ok, I guess that explains why 1.9.2 got broken as there is no nspr header as far as I can see (http://mxr.mozilla.org/mozilla1.9.2/source/content/html/content/src/nsHTMLFrameSetElement.cpp)...unless one of those other headers include a nspr header. If they don't I don't see how it could build successfully with nspr in the tree.
Comment 16 Micah Gersten 2010-08-25 11:57:13 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > It seems that you just need to change nsHTMLFrameSetElement.cpp
> > to include the NSPR header that defines PR_STATIC_ASSERT.
> > 
> > PR_STATIC_ASSERT was added in NSPR 4.7 (see bug 375985).
> > 
> > In NSPR 4.7 - 4.8.4, include "prlog.h".
> 
> Which is how 1.9.1 is
> (http://mxr.mozilla.org/mozilla1.9.1/source/content/html/content/src/nsHTMLFrameSetElement.cpp#46).
> 
> 1.9.1 should be using nspr < 4.8.6, as we didn't take bug 575620 on that
> branch. I don't see why 1.9.1 would suddenly be broken after the fix for bug
> 576447, as that file already includes prlog.h which defines PR_STATIC_ASSERT.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5ad248a9215 fixed 1.9.1, so that's no longer an issue and I'll remove the approval request.

> > 
> > In NSPR 4.8.6 or later, include "prtypes.h". (Since every
> > NSPR header includes "prtypes.h", this is satisfied if
> > you include any NSPR header.)
> 
> Ok, I guess that explains why 1.9.2 got broken as there is no nspr header as
> far as I can see
> (http://mxr.mozilla.org/mozilla1.9.2/source/content/html/content/src/nsHTMLFrameSetElement.cpp)...unless
> one of those other headers include a nspr header. If they don't I don't see how
> it could build successfully with nspr in the tree.

I'm not sure what the answer is here.  Seemingly, an ifdef to include prlog.h if nspr <= 4.8.4 unless you want to bump configure.
Comment 17 Micah Gersten 2010-08-25 11:58:00 PDT
Comment on attachment 463562 [details] [diff] [review]
Bump to 4.8.6

Not needed on 1.9.1 since http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5ad248a9215 fixed it.
Comment 18 Wan-Teh Chang 2010-08-25 12:26:36 PDT
Micah: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5ad248a9215
can also fix the PR_STATIC_ASSERT problem in nsHTMLFrameSetElement.cpp
for 1.9.2.

Is there anything else in 1.9.2 that requires NSPR 4.8.6?  Perhaps NSS 3.12.7?
Comment 19 Wan-Teh Chang 2010-08-25 14:14:28 PDT
Micah: I just did a 1.9.2 build with --with-system-nspr and found
the answer to my question.  NSS 3.12.7, used by mozilla-1.9.2,
requires NSPR 4.8.6 (the new PL_ClearArenaPool function).
Comment 20 Micah Gersten 2010-08-25 14:20:59 PDT
(In reply to comment #19)
> Micah: I just did a 1.9.2 build with --with-system-nspr and found
> the answer to my question.  NSS 3.12.7, used by mozilla-1.9.2,
> requires NSPR 4.8.6 (the new PL_ClearArenaPool function).

Right, but minimum NSS is 3.12.6 still:
http://hg.mozilla.org/releases/mozilla-1.9.2/file/fe24c81faa56/configure.in#l4303
Comment 21 christian 2010-08-31 11:41:08 PDT
So, it sounds like the requirement should be bumped up on the branch, unless we want to deal with the complexity of mixing and matching nss/nspr, their includes, and min required versions...correct?
Comment 22 Micah Gersten 2010-09-06 04:26:20 PDT
Created attachment 472364 [details] [diff] [review]
Add prlog.h to fix the build issue with NSPR less than 4.8.6

What about this patch? This patch was landed on mozilla-1.9.1 originally and fixes the build error with system NSPR less than 4.8.6, for NSPR 4.8.6, it has no effect.  This prevents the bumping of minimum NSPR on the branch.
Comment 23 Wan-Teh Chang 2010-09-07 10:13:39 PDT
Comment on attachment 472364 [details] [diff] [review]
Add prlog.h to fix the build issue with NSPR less than 4.8.6

r=wtc.

Micah, I'm confused what you wanted mozilla-1.9.2.  Do
you want to bump the NSPR version requirement to 4.8.6
(because NSS 3.12.7 requires NSPR 4.8.6), or do you want
to avoid bumping the NSPR version requirement?

Note: the NSS 3.12.6 release notes say that it requires
NSPR 4.8.4.  The release notes usually state a conservative
minimum NSPR version that's higher than necessary, but
it may be a good idea to change bump the NSPR version
requirement to 4.8.4.
Comment 24 Micah Gersten 2010-09-07 13:47:45 PDT
(In reply to comment #23)
> Comment on attachment 472364 [details] [diff] [review]
> Add prlog.h to fix the build issue with NSPR less than 4.8.6
> 
> r=wtc.
> 
> Micah, I'm confused what you wanted mozilla-1.9.2.  Do
> you want to bump the NSPR version requirement to 4.8.6
> (because NSS 3.12.7 requires NSPR 4.8.6), or do you want
> to avoid bumping the NSPR version requirement?
> 
> Note: the NSS 3.12.6 release notes say that it requires
> NSPR 4.8.4.  The release notes usually state a conservative
> minimum NSPR version that's higher than necessary, but
> it may be a good idea to change bump the NSPR version
> requirement to 4.8.4.

The patch is for mozilla-1.9.2 only.  Trunk is unaffected since NSPR was bumped to 4.8.6.  This gets rid of the need to bump NSPR on mozilla-1.9.2.  Sorry for not setting the correct reviewer.  We're running xulrunner-1.9.2 on Hardy with NSPR 4.8 and have not had any issue reported yet.
Comment 25 Micah Gersten 2010-09-07 13:48:43 PDT
Comment on attachment 472364 [details] [diff] [review]
Add prlog.h to fix the build issue with NSPR less than 4.8.6

Removing incorrect review request.
Comment 26 Micah Gersten 2010-09-28 16:17:09 PDT
Actually, now that minimum NSS has been bumped to 3.12.8, NSPR 4.8.6 is now required (bug 575620), so I think now, we should bump minimum NSPR to 4.8.6 on mozilla-1.9.2 now and mozilla-1.9.1 once it lands.
Comment 27 christian 2010-09-28 16:23:47 PDT
Agreed and approved.

Note You need to log in before you can comment on or make changes to this bug.