896704 - Remove the MSVC exemption for FAIL_ON_WARNINGS in media/mtransport

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=5c1ff6c9492b

Comment 1

[Masatoshi Kimura [:emk]]

c:\tools\msvs10\vc\include\errno.h(102) : error C2220: warning treated as error - no 'object' file generated

c:\tools\msvs10\vc\include\errno.h(102) : warning C4005: 'EHOSTUNREACH' : macro redefinition

        e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\media\mtransport\third_party\nrappkit\src\port\win32\include\csi_platform.h(82) : see previous definition of 'EHOSTUNREACH'

Didn't fail locally :(

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: bug896704_nrappkit

Comment 3

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=5f9b558bb684

Comment 4

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

Comment on attachment 833389 [details] [diff] [review]
bug896704_nrappkit

Forwarding review to abr (Adam, you can decide if ekr needs to see this; I don't think so if you're good with it).

Note: don't use splinter to review this patch; it gets confused by the patch format.  Download or perhaps just view in Details.

Comment 5

[Adam Roach [:abr]]

Comment on attachment 833389 [details] [diff] [review]
bug896704_nrappkit

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

r- for unconditional removal of a compatibility feature of nrappkit.

I'm adding Ted to review moz.build, since build system changes need a build peer review.

::: media/mtransport/third_party/nrappkit/src/port/win32/include/csi_platform.h
@@ +76,5 @@
>  #define NR_SOCKET_READ(sock,buf,count)   recv((sock),(buf),(count),0)
>  #define NR_SOCKET_WRITE(sock,buf,count)  send((sock),(buf),(count),0)
>  #define NR_SOCKET_CLOSE(sock)            closesocket(sock)
>  #endif
>  

I suspect this serves a purpose on some platforms, and would expect that nrappkit wants to maintain compatibility with that platform. To that end, we really need the change to be more like:

#ifndef EHOSTUNREACH
#define EHOSTUNREACH WSAEHOSTUNREACH
#endif

You may need to add a "#include <errno.h>" before this set of statements to ensure that errno.h has a chance to define this macro if it's going to.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 833389 [details] [diff] [review]
bug896704_nrappkit

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

Your review as module peer is fine for trivial moz.build changes like this.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Comment 8

[Adam Roach [:abr]]

Comment on attachment 8342337 [details] [diff] [review]
patch v2

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

Looks good to me.

Comment 9

[Masatoshi Kimura [:emk]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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