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

Categories

(Firefox Build System :: General, defect)

x86
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8494715 - Attachment is obsolete: true
Attachment #8515089 - Flags: review?(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!
Attachment #8515089 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/a07e226542b5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.