toolkit/mozapps/update fails to compile with MinGW

RESOLVED FIXED in mozilla33

Status

()

Toolkit
Application Update
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Kathleen Brade, Assigned: Jacek Caban)

Tracking

(Blocks: 1 bug)

Trunk
mozilla33
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8437118 [details] [diff] [review]
MinGW updater fixes

The code under toolkit/mozapps/update does not compile with the mingw-w64 compiler and headers (we encountered this problem doing a 32-bit build for the Tor Browser).  Most of the problems seem to be caused by differences between the Microsoft header files and the MinGW ones.
Attachment #8437118 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 1

4 years ago
(In reply to Kathleen Brade from comment #0)
> Most of the problems seem to be caused by differences
> between the Microsoft header files and the MinGW ones.

Those ideally should be fixed in mingw-w64. I tried to look at them, so I tried building updater myself. I didn't need _WIN32_WINNT changes nor other include changes. What mingw-w64 version are you using?

I will attach the patch I ended with. It's just for error fixes, no warning fixes, just for reference. Also extern "C" changes are probably needed for GCC older than 4.9.
(Assignee)

Comment 2

4 years ago
Created attachment 8438311 [details] [diff] [review]
patch.diff
(Reporter)

Comment 3

4 years ago
(In reply to Jacek Caban from comment #1)
> Those ideally should be fixed in mingw-w64. I tried to look at them, so I
> tried building updater myself. I didn't need _WIN32_WINNT changes nor other
> include changes. What mingw-w64 version are you using?

We were using an older version of mingw-w64.  We switched to gcc 4.8.3 and mingw-w64 3.1.0 and some of the problems did go away.  However, some of the issues remain.

We are cross-compiling on a Linux system and we are mainly working with ESR 24.  We will try cross-compiling mozilla-central in order to confirm that your patch fixes all of the issues.  We will need to also keep the extern "C" for NS_main() for now because we are not ready to jump to gcc 4.9.
(Reporter)

Updated

4 years ago
Attachment #8437118 - Flags: review?(robert.strong.bugs)
(Reporter)

Comment 4

4 years ago
Our mozilla-central build completed successfully using your patch.  Is it too late to include this in Firefox 31?
Comment on attachment 8438311 [details] [diff] [review]
patch.diff

Brian, could I get you to review this? Thanks!
Attachment #8438311 - Flags: review?(netzen)
Attachment #8438311 - Flags: review?(netzen) → review+
(In reply to Kathleen Brade from comment #4)
> Our mozilla-central build completed successfully using your patch.  Is it
> too late to include this in Firefox 31?
After this makes it to mozilla-central we can evaluate whether we can take it for Firefox 31.
Assignee: brade → jacek
Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=c78e93956822

If all goes well I'll land this
c:/builds/moz2_slave/try-w64-0000000000000000000000/build/toolkit/mozapps/update/updater/updater.cpp(2988) : error C2589: '(' : illegal token on right side of '::'
c:/builds/moz2_slave/try-w64-0000000000000000000000/build/toolkit/mozapps/update/updater/updater.cpp(2988) : error C2059: syntax error : '::' 

:(
Is that on the max call? #undef max might help at the top?
(Assignee)

Comment 10

4 years ago
I guess we should define NOMINMAX for Windows builds. That prevents windef.h from defining min/max macros. I will send a fixed version to try server soon.
(Assignee)

Comment 11

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7750c3d7a313
(Assignee)

Comment 12

4 years ago
I pushed wrong version. Here is the right one:

https://tbpl.mozilla.org/?tree=Try&rev=7cfb7bbd3e11
Still busted. I think bbondy may be correct in comment #9 as well
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_win.cpp#40
I ran into a similar problem with sandboxing, if I recall correctly there was an implicit compile time include specified that was including windows early.  So #undef'ing the max macro worked for me.
(Assignee)

Comment 15

4 years ago
The problem from the last push is quite different. Using NOMINMAX, max is not defined. However, this can't be found now. It's declared in algorithm header. The version that I accidentally pushed had it included (although missed NOMINMAX part) and that caused another error:

#error : "STL code can only be used with infallible ::operator new()"

I may take a look at this later, but maybe someone knows the solution off-hand?
Try setting DISABLE_STL_WRAPPING = True in the moz.build file under the conditions for mingw
(Assignee)

Comment 17

4 years ago
This works, thanks:

https://tbpl.mozilla.org/?tree=Try&rev=adce3dc47f89

I will land it tomorrow unless you think that needs a new review.
(Assignee)

Comment 18

4 years ago
Sorry for the delay.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bbedf6937aa
https://hg.mozilla.org/mozilla-central/rev/7bbedf6937aa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1034127
Comment on attachment 8438311 [details] [diff] [review]
patch.diff

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

::: toolkit/mozapps/update/common/updatehelper.cpp
@@ -10,4 @@
>  #define _WIN32_WINNT 0x602
>  #include <objbase.h>
>  #include <shobjidl.h>
> -#pragma comment(lib, "ole32.lib")

Removing this line broke --enable-metro builds (bug 1034127).  Since this line is inside an #ifdef MOZ_METRO block, it should have no effect on default builds.  Can we restore this line?
(Assignee)

Comment 21

4 years ago
I'm sorry for intoducing this. I'm away on vacations now so I can't prepare a patch. Moving it to moz.build faile would be even nicer than restoring the file. I will do that on Thursday.
(Assignee)

Comment 22

4 years ago
I pushed a partial backout:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d179aa6e46f

I decided that moving it to moz.build file may wait for bug 969321 and this way I could push the fix right away.
https://hg.mozilla.org/mozilla-central/rev/6d179aa6e46f
You need to log in before you can comment on or make changes to this bug.