Closed Bug 1072296 Opened 5 years ago Closed 5 years ago
webrtc code defines WINVER and _WIN32
_WINNT differently than mozilla-config .h .in, leading to warnings
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.
The simplest thing that could possibly work.
Attachment #8494715 - Flags: review?(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.
Attachment #8494715 - Flags: review?(mshal) → review+
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.
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 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!
Attachment #8515089 - Flags: review?(mshal) → review+
Assignee: nobody → nfroyd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.