816822 - Android build patches for WebRTC unit tests

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: make android unit tests build, patch 1, v1 (WIP)

I didn't get as much time to poke at this today as I was hoping.

This patch is a hack that makes the signaling tests build; my suspicion is that the right fix there will not touch gtest-port.h but will do the appropriate override somewhere at a higher level.

Still need to fix the mtransport tests.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I think we can just do -DGTEST_HAS_TR1_TUPLE=0 on Android.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Fix unit tests on Android.

Cleaner fix for dmose's issue. Also fixes all remaining issues.

IIRC in one of the earlier patches I made Android set Userspace_os_Linux and added ifdef ANDROID's, this code is doing the opposite. Should probably clean this up to make all patches uniform.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v3, tests to make C++ unit tests build on Android

Something on mozilla-inbound caused this patch to bit-rot.  Here's an updated version that applies.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 687931 [details] [diff] [review]
Patch 1, v3, tests to make C++ unit tests build on Android

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

::: media/mtransport/test/sctp_unittest.cpp
@@ +97,5 @@
>      EXPECT_GE(r, 0);
>  
>      memset(&local_addr_, 0, sizeof(local_addr_));
>      local_addr_.sconn_family = AF_CONN;
> +#if !defined(__Userspace_os_Linux) && !defined(__Userspace_os_Windows) && !defined(__Userspace_os_Android)

As per before, maybe this wants to just be defined(__Userspace_os_Darwin) ?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v4, fixes to make C++ unit tests build on Android

The old patch was bitrotted by some recent checkin.  Here's an unbitrotted version, with feedback+ and review? carried forward.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 691094 [details] [diff] [review]
Patch 1, v4, fixes to make C++ unit tests build on Android

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

This patch looks good, my only question is about the license on the nrappkit files.  The license doesn't actually state what license it is, which makes it harder to analyse.  

If these are just copies of other files in the tree, and [e-mail] has already OKed this license, r=dmose.  If not, can you either elaborate on why we don't need approval, or else mail them for approval?

r=dmose conditional on the licensing stuff being sorted out

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Oh, this also still needs the tweak the Ted requested in comment 5 before it lands.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>If these are just copies of other files in the tree

They are copies of the already-present linux/ files, with small fixes for Android.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Landed on mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbd7e9882b3

Setting in-testsuite-, because this is a build fix, so the compiler itself acts as an automated test.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5bbd7e9882b3