567620 - Bump minimum required version for system NSPR to 4.8.6

Status: RESOLVED FIXED

Description

[Micah Gersten]

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]

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]

(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]

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]

(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]

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]

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]

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]

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]

(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)]

http://hg.mozilla.org/mozilla-central/rev/4cdc47a945a7

Comment 11

[christian]

Micah, a build log would be great so we can see if it is needed on the branches.

Comment 12

[Micah Gersten]

(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

[christian]

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]

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]

(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]

(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]

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]

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]

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]

(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]

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]

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]

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]

(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]

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]

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]

Agreed and approved.

Comment 28

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5c88bd06125
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/074eeb698869