The default bug view has changed. See this FAQ.

Bump minimum required version for system NSPR to 4.8.6

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Micah Gersten, Assigned: Micah Gersten)

Tracking

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 .11-fixed, status1.9.1 .14-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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
Attachment #446942 - Flags: review?(benjamin)
(Assignee)

Updated

7 years ago
Assignee: nobody → mozilla-bugs
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.
(Assignee)

Comment 2

7 years ago
(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.
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.
blocking2.0: --- → final+
Attachment #446942 - Flags: review?(benjamin) → review-
(Assignee)

Comment 4

7 years ago
(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.
(Assignee)

Comment 5

7 years ago
Created attachment 463562 [details] [diff] [review]
Bump to 4.8.6

Bump to 4.8.6 after landing of bug 575620
Attachment #446942 - Attachment is obsolete: true
Attachment #463562 - Flags: review?(benjamin)
Attachment #463562 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #463562 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Blocks: 575620
(Assignee)

Comment 6

7 years ago
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
Attachment #463562 - Flags: approval1.9.2.9?
Attachment #463562 - Flags: approval1.9.1.12?
(Assignee)

Updated

7 years ago
No longer blocks: 575620

Comment 7

7 years ago
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.
Attachment #463562 - Flags: approval1.9.2.9?
Attachment #463562 - Flags: approval1.9.2.9+
Attachment #463562 - Flags: approval1.9.1.12?
Attachment #463562 - Flags: approval1.9.1.12+

Updated

7 years ago
Attachment #463562 - Flags: approval1.9.2.9?
Attachment #463562 - Flags: approval1.9.2.9+
Attachment #463562 - Flags: approval1.9.1.12?
Attachment #463562 - Flags: approval1.9.1.12+

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
(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.
http://hg.mozilla.org/mozilla-central/rev/4cdc47a945a7
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 11

7 years ago
Micah, a build log would be great so we can see if it is needed on the branches.
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> Micah, a build log would be great so we can see if it is needed on the
> branches.

http://launchpadlibrarian.net/53631071/buildlog_ubuntu-hardy-i386.xulrunner-1.9.1_1.9.1.12~hg20100813r27046%2Bnobinonly-0ubuntu1~umd1~hardy_FAILEDTOBUILD.txt.gz

http://launchpadlibrarian.net/53628458/buildlog_ubuntu-hardy-i386.xulrunner-1.9.2_1.9.2.9~hg20100813r34524%2Bnobinonly-0ubuntu1~umd1~hardy_FAILEDTOBUILD.txt.gz

Hardy has 4.8

Comment 13

7 years ago
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

7 years ago
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

7 years ago
(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.
(Assignee)

Comment 16

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
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.
Attachment #463562 - Flags: approval1.9.1.12?

Comment 18

7 years ago
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?
OS: Linux → Windows CE

Updated

7 years ago
OS: Windows CE → Linux

Comment 19

7 years ago
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).
(Assignee)

Comment 20

7 years ago
(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

7 years ago
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?
(Assignee)

Comment 22

7 years ago
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.
Attachment #472364 - Flags: review?(ted.mielczarek)
Attachment #472364 - Flags: approval1.9.2.10?

Comment 23

7 years ago
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.
Attachment #472364 - Flags: review+
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 25

7 years ago
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.
Attachment #472364 - Flags: review?(ted.mielczarek) → review?

Updated

7 years ago
Attachment #472364 - Flags: approval1.9.2.11? → approval1.9.2.11+
Keywords: checkin-needed

Updated

7 years ago
Attachment #463562 - Flags: approval1.9.2.9?
(Assignee)

Comment 26

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #463562 - Flags: approval1.9.2.11?
Attachment #463562 - Flags: approval1.9.1.14?

Updated

7 years ago
Attachment #472364 - Flags: approval1.9.2.11+ → approval1.9.2.11-
Attachment #472364 - Flags: review?

Updated

7 years ago
Attachment #463562 - Flags: approval1.9.2.11?
Attachment #463562 - Flags: approval1.9.2.11+
Attachment #463562 - Flags: approval1.9.1.14?
Attachment #463562 - Flags: approval1.9.1.14+

Comment 27

7 years ago
Agreed and approved.
Summary: mozilla-central won't build with the latest system NSPR (4.8.4) → Bump minimum required version for system NSPR to 4.8.6
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5c88bd06125
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/074eeb698869
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → .14-fixed
status1.9.2: --- → .11-fixed
Keywords: checkin-needed

Updated

7 years ago
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---

Updated

7 years ago
Attachment #472364 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.