Closed Bug 837395 Opened 7 years ago Closed 7 years ago

Windows build fails in nrappkit under VC9 because EAFNOSUPPORT is undefined

Categories

(Core :: WebRTC: Networking, defect, major)

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: wgianopoulos, Assigned: wgianopoulos)

References

Details

(Keywords: regression, Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(1 file, 2 obsolete files)

This is a regression from bug 825927, but I can't set it as such because that is a security bug.

The actual compiler error is:

c:/Users/wag/mozilla/mozilla2/fx-obj/media/mtransport/third_party/nrappkit/nrappkit_nrappkit/../../../../../../media/mtransport/third_party/nrappkit/src/util/util.c(576) : error C2065: 'EAFNOSUPPORT' : undeclared identifier
Attached patch workaround (obsolete) — Splinter Review
This allows the build to complete, although probably not the correct approach.
Comment on attachment 709378 [details] [diff] [review]
workaround

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

::: media/mtransport/third_party/nrappkit/src/util/util.c
@@ +513,5 @@
>  #include <sys/socket.h>
>  #endif
> +#ifndef EAFNOSUPPORT
> +#define EAFNOSUPPORT            WSAEAFNOSUPPORT
> +#endif

Please move this inside the #ifdef WIN32
Attachment #709378 - Flags: review+
Seems reasonable to me; you could check with ted or a build peer, or commit
Whiteboard: [WebRTC], [blocking-webrtc+]
Comment on attachment 709378 [details] [diff] [review]
workaround

I do think this is as close we're going to get to ”the right approach” as we can. In a quick survey of how other projects have solved this problem, substituting WSA* equivalents for missing errno codes seems to be the most common approach.

The actual errno codes returned from inet_ntop are specified by POSIX, so we're not really at liberty to select a different code. 

The one tweak I would suggest is moving the "#ifndef ENOAFSUPPORT" block inside the "#ifdef WIN32" block, as it won't ever apply to other builds.
 
(I was in the process of R+'ing this, but it looks like Jesup beat me to it)
Ideally we should have a header file somewhere central where we can do something like this:
http://mxr.mozilla.org/mozilla-central/source/netwerk/sctp/src/netinet/sctp_os_userspace.h#139
(In reply to Philip Chee from comment #5)
> Ideally we should have a header file somewhere central where we can do
> something like this:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/sctp/src/netinet/
> sctp_os_userspace.h#139

The problem is that this is part of nrappkit, which is 3rd party code. We upstream all our changes back to the original project, so relying on code elsewhere in the Mozilla tree isn't feasible.
Attached patch workaround v2 (obsolete) — Splinter Review
Actually, it is probably better to put this inside the #ifdef win32.
Attachment #709391 - Flags: review?(rjesup)
OIC you already said move it inside the ifdef.  I will post a check-in ready patch.
Assignee: nobody → bill
Status: NEW → ASSIGNED
Attachment #709391 - Flags: review?(rjesup)
Attachment #709391 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #709378 - Attachment is obsolete: true
Comment on attachment 709394 [details] [diff] [review]
check-in ready patch

This version looks ready for checkin.
Attachment #709394 - Flags: review+
(In reply to Adam Roach [:abr] from comment #10)
> Comment on attachment 709394 [details] [diff] [review]
> check-in ready patch
> 
> This version looks ready for checkin.

Great that makes it clearer for those who do the checkin-needed bugs.  I don't have checkin priv's.
Comment on attachment 709394 [details] [diff] [review]
check-in ready patch

We've mostly been having Randell do our check-ins. I've tagged him with a checkin request on this patch.
Attachment #709394 - Flags: checkin?(rjesup)
I'll get it committed later today
Attachment #709394 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/eae4b34eb792
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in before you can comment on or make changes to this bug.