Closed Bug 793393 Opened 12 years ago Closed 12 years ago

Can no longer build mozilla-central under windows using MSVC 9

Categories

(Core :: WebRTC: Networking, defect)

Product:
Core
Component:
WebRTC: Networking
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: wgianopoulos, Assigned: jesup)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

compile errors from my build
238.71 KB, text/plain
Details
Modify libsrtp to let us force StandardInteger.h
2.87 KB, patch
khuey
: review+
Details | Diff | Splinter Review 
VC8, VC9 and VC10 are all listed a supported build platforms for mozilla-centreal, yet since the landing of bug 792325 I can no longer build using VC9.  It seems to be a uint64_t versus uint64 type issue.
Attachment #663688 - Attachment description: compile errors form my build → compile errors from my build
Would it be possible to get that backed out and fixed on alder?
This should fix it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/030b4a47baa4
Assignee: nobody → ehsan
Target Milestone: --- → mozilla18
Even if it fixes it for older MSVC versions, it breaks the build in versions that actually run in our automation.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cec9d5075819
Well needs to work in vc10 Vc 9 and vc 8 because they are all listed as supported build environments.
tested on VC9 (32-bit, can't run the 64-bit) and VC10/VC10x64
Comment on attachment 663787 [details] [diff] [review]
Fix HAVE_64BIT_OS test, and force define of uint64_t on win x32 to fix VC8/VC9

Looking for review to fix VC8/VC9; I want to see a clean try (see above) and have another dev verify (especially vc9 x64, which I can't run) before I'll check this in.

Ted or eshan or whomever: feel free to step up and review instead! :-)
Attachment #663787 - Flags: review?(khuey)
Depends on: 290518, 793465
Comment on attachment 663787 [details] [diff] [review]
Fix HAVE_64BIT_OS test, and force define of uint64_t on win x32 to fix VC8/VC9

FYI this doesn't work on VC9x64.
Attached patch Possible patch (obsolete) — — Splinter Review 
This works for me on VC9x64 but I haven't tried VC9x32 VC10x64 or VC10x32 yet.
(In reply to neil@parkwaycc.co.uk from comment #10)
> Created attachment 663806 [details] [diff] [review]
> Possible patch
> 
> This works for me on VC9x64 but I haven't tried VC9x32 VC10x64 or VC10x32
> yet.

I just fired off a vc9x32 build.  I will report back.
(In reply to neil@parkwaycc.co.uk from comment #10)
> Created attachment 663806 [details] [diff] [review]
> Possible patch
> 
> This works for me on VC9x64 but I haven't tried VC9x32 VC10x64 or VC10x32
> yet.
This does NOT work for VC9x32.  It does not end up defining FORCE_UINT64_T=1.
I think the issue is logical because it is an ifneq.

I think for the non 64-bit case this is doing the define only for the cases where we don't want to.

It would probably also be better to do a test for LT the VC10 version rather than make it work for 2 specific version values.
So i think what is required is not 64-bit and version lt 1600.
Well looking at other Makefiles it would seem we do this all over the place so although going on lt 1600 might be more correct, the testing for 1400 and 1500 should work.  I am trying a build changing the test to filter-out.
Comment on attachment 663806 [details] [diff] [review]
Possible patch

>+ifneq ($(HAVE_64BIT_OS),$(filter 1400 1550, $(_MSC_VER)))

to be clear, what I am testing now under VC9x32 is changing the above line to:

ifneq ($(HAVE_64BIT_OS),$(filter-out 1400 1550, $(_MSC_VER)))

This needs to be tested at least under VC10x32 and VC10x64, neither of which I have the facilities to do.
(In reply to Bill Gianopoulos [:WG9s] from comment #16)
> Comment on attachment 663806 [details] [diff] [review]
> Possible patch
> 
> >+ifneq ($(HAVE_64BIT_OS),$(filter 1400 1550, $(_MSC_VER)))
> 
> to be clear, what I am testing now under VC9x32 is changing the above line
> to:
> 
> ifneq ($(HAVE_64BIT_OS),$(filter-out 1400 1550, $(_MSC_VER)))
> 
> This needs to be tested at least under VC10x32 and VC10x64, neither of which
> I have the facilities to do.

With that change I was able to successfully build VC9x32.
Comment on attachment 663877 [details] [diff] [review]
Fix HAVE_64BIT_OS test, and force define of uint64_t on win x32 to fix VC8/VC9

Checked the try push - it builds, but the test at the top that was modified is wrong; MSC_VER shouldn't be part of deciding what the length of a LONG is; my patch had it correct (only test HAVE_64BIT_OS). I'll upload a corrected patch shortly.
Attachment #663877 - Attachment is obsolete: true
Assignee: ehsan → rjesup
Ok, this is the *right* way to do it - use mozilla/StandardInteger.h everywhere, via a patch to libsrtp that's fine for upstreaming (lets configure force a stdint replacement).  Gets rid of the whole FORCE_UINT64_T, etc.
Attachment #663901 - Attachment description: Fix HAVE_64BIT_OS test, and modify libsrtp to let us force StandardInteger.h → Modify libsrtp to let us force StandardInteger.h
Attachment #663901 - Flags: review?(khuey)
Attachment #663879 - Attachment is obsolete: true
Attachment #663787 - Attachment is obsolete: true
Attachment #663787 - Flags: review?(khuey)
Attachment #663806 - Attachment is obsolete: true
Comment on attachment 663901 [details] [diff] [review]
Modify libsrtp to let us force StandardInteger.h

Review of attachment 663901 [details] [diff] [review]:
-----------------------------------------------------------------

r=me presuming it actually works.
Attachment #663901 - Flags: review?(khuey) → review+
https://tbpl.mozilla.org/?tree=Try&rev=bac64cd7b1a1 looks like it will be all green (linux opt is an infra thing hitting everyone).  A local VC9 x32 build was fine.  Just need a VC9 x64 build and I'll commit.
jessup asked for feedback on #developers:  FWIW, vs2008x64 builds for me both with and without this patch.
(In reply to Mark Hammond from comment #24)
> jessup asked for feedback on #developers:  FWIW, vs2008x64 builds for me
> both with and without this patch.
vc9x64 builds for me with attachment 663901 [details] [diff] [review] but not unpatched.

(In reply to Bill Gianopoulos from comment #16)
> >+ifneq ($(HAVE_64BIT_OS),$(filter 1400 1550, $(_MSC_VER)))
> ifneq ($(HAVE_64BIT_OS),$(filter-out 1400 1550, $(_MSC_VER)))
Yeah, you were probably right about this.

(In reply to Randell Jesup from comment #19)
> MSC_VER shouldn't be part of deciding what the length of a LONG is
VC is not LP64, it is LLP64; LONG is always 32-bit. So yes, it does decide.
(In reply to neil@parkwaycc.co.uk from comment #26)
> (In reply to Mark Hammond from comment #24)
> > jessup asked for feedback on #developers:  FWIW, vs2008x64 builds for me
> > both with and without this patch.
> vc9x64 builds for me with attachment 663901 [details] [diff] [review] but
> not unpatched.
> 
> (In reply to Bill Gianopoulos from comment #16)
> > >+ifneq ($(HAVE_64BIT_OS),$(filter 1400 1550, $(_MSC_VER)))
> > ifneq ($(HAVE_64BIT_OS),$(filter-out 1400 1550, $(_MSC_VER)))
> Yeah, you were probably right about this.

Except VC9 is 1500 not 1550...  (per all the other makefiles in our system apparently, and probably why I had problems with that patch).  No longer relevant, now proxied to the StandardInteger.h support.

> (In reply to Randell Jesup from comment #19)
> > MSC_VER shouldn't be part of deciding what the length of a LONG is
> VC is not LP64, it is LLP64; LONG is always 32-bit. So yes, it does decide.

Ah.  Didn't know that.  In any case, not an issue now.
https://hg.mozilla.org/mozilla-central/rev/718d68677a18
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: