Build error in accessible/public/msaa on x64 Windows with VC8

RESOLVED FIXED in mozilla1.9.2a1

Status

defect
RESOLVED FIXED
11 years ago
Last year

People

(Reporter: emk, Assigned: emk)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
x86_64
Windows Vista
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 5 obsolete attachments)

You will fail to build Firefox with the following message:
   Creating library AccessibleMarshal.lib and object AccessibleMarshal.exp
dlldata.obj : error LNK2001: unresolved external symbol _ISimpleDOMText_ProxyFileInfo
dlldata.obj : error LNK2001: unresolved external symbol _ISimpleDOMNode_ProxyFileInfo
dlldata.obj : error LNK2001: unresolved external symbol _ISimpleDOMDocument_ProxyFileInfo
AccessibleMarshal.dll : fatal error LNK1120: 3 unresolved externals
Posted patch patch (obsolete) — Splinter Review
I also removed old workaround. (see bug 471451.)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #355095 - Flags: review?(ted.mielczarek)
Attachment #355095 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 355095 [details] [diff] [review]
patch

Could you import timeless' patch from bug 471451 and make your patch apply on top of that? As written your patch will still trip his bug.
Also, can you check with Makoto about this? He has a bug on making the Windows x64 build work, and your "-env win32" switch might be a problem for him.
Posted patch Integrate bug 471451 patch (obsolete) — Splinter Review
I removed midl stuff from js/src/configure.in completely per bug 471451 comment #3.
Kato-san, do I have to check for x86_64 target?
Attachment #355095 - Attachment is obsolete: true
Attachment #355134 - Flags: review?(m_kato)
Attachment #355134 - Flags: review?(m_kato) → review-
Comment on attachment 355134 [details] [diff] [review]
Integrate bug 471451 patch

You should add it when target is i*86-* only.
Posted patch Added platform check (obsolete) — Splinter Review
I found that it was not required to check whether string is empty because |=| will perform a string compare rather than an arithmetic compare.
Attachment #355134 - Attachment is obsolete: true
Attachment #355463 - Flags: review?(m_kato)
Kimura-san, can we remove no_robut options?  (anyone may use old version of MIDL)

And I believe MIDL version 7 (or maybe from VC7.1?) supports /env win32 options, so if MIDL is version 7, we must always add it, I think.
(In reply to comment #7)
> Kimura-san, can we remove no_robut options?  (anyone may use old version of
> MIDL)
See bug 378928. (I thought I referred it in comment #1. The bug number was wrong...)
Although older midl will generate no_robust proxy/stub, it is harmless.

> And I believe MIDL version 7 (or maybe from VC7.1?) supports /env win32
> options, so if MIDL is version 7, we must always add it, I think.
I can build Firefox using VC9 without /env win32 switch.
I believe VC8 has a bug because midl should generate x86 compatible proxy/stub if the build environment is configured for x86 target. Therefore I wrote a patch as a workaround for specific midl version.
Attachment #355463 - Flags: review?(m_kato) → review+
Comment on attachment 355463 [details] [diff] [review]
Added platform check

Agree
Posted patch sync to trunk (obsolete) — Splinter Review
Thank you, Kato-san.
Asking build-config owner for review again.
The patch becomes obsolete since the fix of bug 471451 landed. I have updated a patch.
Attachment #355463 - Attachment is obsolete: true
Attachment #355949 - Flags: review?(ted.mielczarek)
Depends on: 471451
Do the 4 other lines in /js/src/configure.in need to be kept ?

Comm-central will want to stay in sync, I guess.
Version: unspecified → Trunk
> Do the 4 other lines in /js/src/configure.in need to be kept ?
Removed.
> Comm-central will want to stay in sync, I guess.
Plaease file a separate bug for comm-central.
Attachment #355949 - Attachment is obsolete: true
Attachment #355989 - Flags: review?(ted.mielczarek)
Attachment #355949 - Flags: review?(ted.mielczarek)
Comment on attachment 355989 [details] [diff] [review]
Removed from js/src/configure.in completely

+            if test \( "$_MIDL_MAJOR_VERSION" = "7" -a "$_MIDL_MINOR_VERSION" = "00" -a "$_MIDL_REV_VERSION" = "0499" \); then

I trust that this line won't regress timeless' bug if midl is not found? (It looks like you're only doing string comparisions, so I hope so.)

Can you also remove MIDL/MIDL_FLAGS from js/src/config/autoconf.mk.in?
Attachment #355989 - Flags: review?(ted.mielczarek) → review+
Carrying forward r+.
(In reply to comment #13)
> I trust that this line won't regress timeless' bug if midl is not found? (It
> looks like you're only doing string comparisions, so I hope so.)
Honestly, I have no idea how to make a midl-less build environment.
Both VC8 and VC9 are shipped with midl. Windows SDK also has. MinGW build environment won't even execute the codepath in question. I doubt we need to support a midl-less MSVC environment in the first place.
Having said that, my patch should not regress the issue.
> Can you also remove MIDL/MIDL_FLAGS from js/src/config/autoconf.mk.in?
Done.
Attachment #355989 - Attachment is obsolete: true
Attachment #356727 - Flags: review+
Attachment #356727 - Attachment description: Reveved midl stuff from autoconf.mk.in → Removed midl stuff from autoconf.mk.in
Keywords: checkin-needed
Comment on attachment 356727 [details] [diff] [review]
Removed midl stuff from autoconf.mk.in
[Checkin: Comment 15]


http://hg.mozilla.org/mozilla-central/rev/24a0ddf8cdfa
Attachment #356727 - Attachment description: Removed midl stuff from autoconf.mk.in → Removed midl stuff from autoconf.mk.in [Checkin: Comment 15]
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
(untested) trivial copy.


(In reply to comment #12)
> > Comm-central will want to stay in sync, I guess.
> Plaease file a separate bug for comm-central.

It's easier to keep both in the same bug, I think.
Attachment #357968 - Flags: review?(kairo)
Comment on attachment 357968 [details] [diff] [review]
(Bv1) Sync comm-central after (m-c) bug 471451 and bug 471854
[Checkin: Comment 19]

I also would have liked a separate bug better, but here it goes. I can't test this, so r=me by looking at the code and not spotting anything that looks wrong to me. I trust what was reviewed for Mozilla should work for us as well.
Attachment #357968 - Flags: review?(kairo) → review+
Posted patch branch patchSplinter Review
I want to land this on the branch for comm-central.
The risk is very low.
Attachment #358031 - Flags: approval1.9.1?
Comment on attachment 357968 [details] [diff] [review]
(Bv1) Sync comm-central after (m-c) bug 471451 and bug 471854
[Checkin: Comment 19]


http://hg.mozilla.org/comm-central/rev/1a9b710f8ea6
Attachment #357968 - Attachment description: (Bv1) Sync comm-central after (m-c) bug 471451 and bug 471854 → (Bv1) Sync comm-central after (m-c) bug 471451 and bug 471854 [Checkin: Comment 19]
Flags: in-testsuite-
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #358031 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 1.9.1 landing]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.