1022847 - toolkit/mozapps/update fails to compile with MinGW

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Kathleen :Brade]

Attachment: 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.

Comment 1

[Jacek Caban]

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

Comment 2

[Jacek Caban]

Attachment: patch.diff

Comment 3

[Kathleen :Brade]

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

Comment 4

[Kathleen :Brade]

Our mozilla-central build completed successfully using your patch.  Is it too late to include this in Firefox 31?

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 8438311 [details] [diff] [review]
patch.diff

Brian, could I get you to review this? Thanks!

Comment 6

[Robert Strong (they/them - no direct email)]

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

Comment 7

[Robert Strong (they/them - no direct email)]

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=c78e93956822

If all goes well I'll land this

Comment 8

[Robert Strong (they/them - no direct email)]

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 : '::' 

:(

Comment 9

[Brian R. Bondy [:bbondy]]

Is that on the max call? #undef max might help at the top?

Comment 10

[Jacek Caban]

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.

Comment 11

[Jacek Caban]

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

Comment 12

[Jacek Caban]

I pushed wrong version. Here is the right one:

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

Comment 13

[Robert Strong (they/them - no direct email)]

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

Comment 14

[Brian R. Bondy [:bbondy]]

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.

Comment 15

[Jacek Caban]

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?

Comment 16

[Brian R. Bondy [:bbondy]]

Try setting DISABLE_STL_WRAPPING = True in the moz.build file under the conditions for mingw

Comment 17

[Jacek Caban]

This works, thanks:

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

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

Comment 18

[Jacek Caban]

Sorry for the delay.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bbedf6937aa

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bbedf6937aa

Comment 20

[Matt Brubeck (:mbrubeck)]

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?

Comment 21

[Jacek Caban]

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.

Comment 22

[Jacek Caban]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d179aa6e46f