837395 - Windows build fails in nrappkit under VC9 because EAFNOSUPPORT is undefined

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Bill Gianopoulos [:WG9s]]

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

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: workaround

This allows the build to complete, although probably not the correct approach.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Seems reasonable to me; you could check with ted or a build peer, or commit

Comment 4

[Adam Roach [:abr]]

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)

Comment 5

[Philip Chee]

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

Comment 6

[Adam Roach [:abr]]

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

Comment 7

[Bill Gianopoulos [:WG9s]]

Attachment: workaround v2

Actually, it is probably better to put this inside the #ifdef win32.

Comment 8

[Bill Gianopoulos [:WG9s]]

OIC you already said move it inside the ifdef.  I will post a check-in ready patch.

Comment 9

[Bill Gianopoulos [:WG9s]]

Attachment: check-in ready patch

Comment 10

[Adam Roach [:abr]]

Comment on attachment 709394 [details] [diff] [review]
check-in ready patch

This version looks ready for checkin.

Comment 11

[Bill Gianopoulos [:WG9s]]

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

[Adam Roach [:abr]]

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.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

I'll get it committed later today

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[Phil Ringnalda (:philor)]

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