1072296 - webrtc code defines WINVER and _WIN32_WINNT differently than mozilla-config.h.in, leading to warnings

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

Compiling for Windows nets a fair amount of these warnings:

33:33.97 Warning: C4005 in c:\users\froydnj\mozilla-central\obj-x86_64-pc-mingw32\dist\include\mozilla-config.h: 'WINVER' : macro redefinition
33:33.97 c:\users\froydnj\mozilla-central\obj-x86_64-pc-mingw32\media\webrtc\signalingtest\signaling_ecc\../../../../dist/include/mozilla-config.h(131) : warning C4005: 'WINVER' : macro redefinition
33:33.97         command-line arguments :  see previous definition of 'WINVER'
33:33.97 Warning: C4005 in c:\users\froydnj\mozilla-central\obj-x86_64-pc-mingw32\dist\include\mozilla-config.h: '_WIN32_WINNT' : macro redefinition
33:33.97 c:\users\froydnj\mozilla-central\obj-x86_64-pc-mingw32\media\webrtc\signalingtest\signaling_ecc\../../../../dist/include/mozilla-config.h(141) : warning C4005: '_WIN32_WINNT' : macro redefinition
33:33.97         command-line arguments :  see previous definition of '_WIN32_WINNT'

due to this bit of webrtc configury:

http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/build/common.gypi#3321

My inclination is to forcibly unset these two defines in mozilla-config.h.in prior to @ALL_DEFINES@; I think that means we will complain if anything in WebRTC uses things that can't be supported on Windows XP?  Alternatively, we could remember the old values, run @ALL_DEFINES@, and then put the values back.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: undefine WINVER and _WIN32_WINNT in mozilla-config.h.in

The simplest thing that could possibly work.

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8494715 [details] [diff] [review]
undefine WINVER and _WIN32_WINNT in mozilla-config.h.in

I am a bit concerned about this since the WINVER & _WIN32_WINNT defines from gyp are readily apparent on the command-line, so if someone is debugging code dependent on those values, they'll see those defines, and likely be very confused that they are sneakily unset and re-defined by mozilla-config.h

I'd rather remove them from common.gypi entirely to avoid this confusion, but since that's from upstream I guess that will be more annoying to do.

Another option might be to change @ALLDEFINES@ to do #ifndef/#define/#endif instead of just a straight #define, though that would make configure values no longer the sole source of truth, and possibly have other side-effects. Might be worth checking with glandium on this.

In any case, with this patch, I think it would be helpful to identify that the command-line values come from gyp. Just from reading the patch, I thought it had something to do with overriding the Visual Studio environment.

Comment 3

[(not currently active) Ted Mielczarek]

There's a build_with_mozilla gyp variable that we use elsewhere, we could have common.gypi only set these if that == 0:
http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/media/webrtc/trunk/build/common.gypi#l954

That should be upstreamable just fine.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: make webrtc only define WINVER and _WIN32_WINNT if we're not building for mozilla

This version of the patch implements Ted's suggestion by conditionally defining
things in gyp.  This is nicer from the upstreamability standpoint as well as
not making mozilla-config.h.in do sneaky things.

Verified that it eliminates the macro redefinition warnings while building webrtc.

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8515089 [details] [diff] [review]
make webrtc only define WINVER and _WIN32_WINNT if we're not building for mozilla

Looks good to me!

Comment 6

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a07e226542b5

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a07e226542b5