Closed Bug 813911 Opened 9 years ago Closed 9 years ago

nrappkit code doesn't yet build on Android

Categories

(Core :: WebRTC, defect, P2)

All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: gcp, Assigned: gcp)

References

Details

(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch 1. Fix nrappkit build. (obsolete) — Splinter Review
Fix nrappkit so it compiles on android.
Attachment #683942 - Attachment is patch: true
Assignee: nobody → gpascutto
Blocks: 750869
Priority: -- → P2
Whiteboard: [WebRTC] [blocking-webrtc-]
Attachment #683942 - Flags: review?(dmose)
Attachment #683942 - Flags: feedback?(ted)
Comment on attachment 683942 [details] [diff] [review]
Patch 1. Fix nrappkit build.

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

::: media/mtransport/third_party/nrappkit/nrappkit.gyp
@@ +194,5 @@
>                	      './src/port/win32/include/csi_platform.h',
>  		 ],
>                }],
> +              ## Android (Linux)
> +              [ 'OS == "android"', {

Can't you just combine this into the Linux block above, by changing it to
[ 'OS == "linux" or OS == "android"'. {

@@ +200,5 @@
> +                    '-Wall',
> +                    '-Wno-parentheses',
> +                    '-Wno-strict-prototypes',
> +                    '-Wmissing-prototypes',
> +                 ],

We don't actually use the cflags here, do we? We only use cflags_mozilla, right?
Attachment #683942 - Flags: feedback?(ted) → feedback-
Comment on attachment 683942 [details] [diff] [review]
Patch 1. Fix nrappkit build.

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

::: media/mtransport/third_party/nrappkit/nrappkit.gyp
@@ +200,5 @@
> +                    '-Wall',
> +                    '-Wno-parentheses',
> +                    '-Wno-strict-prototypes',
> +                    '-Wmissing-prototypes',
> +                 ],

Ted's correct: cflags is ignored; it needs to be cflags_mozilla

@@ +218,5 @@
>  
> +		 'include_dirs': [
> +		     'src/port/linux/include'
> +		 ],
> +		 

Spurious spaces
Attached patch Patch 1.v2 Fix nrappkit build. (obsolete) — Splinter Review
Incorporate review comments.
Attachment #683942 - Attachment is obsolete: true
Attachment #683942 - Flags: review?(dmose)
Attachment #690429 - Flags: review?(dmose)
Comment on attachment 690429 [details] [diff] [review]
Patch 1.v2 Fix nrappkit build.

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

Separately from the android case thing, this blows up on r5c because it can't find <rpc/rpc.h>.  I'll upload a hack fix in a minute that simply disables that on both Android and Linux.

::: media/mtransport/third_party/nrappkit/nrappkit.gyp
@@ +197,2 @@
>                ## Linux
> +              [ '(OS == "linux") or (OS == "Android")', {

android needs to be lower-case, which makes this iteration of the patch not work at all.  In order to catch issues like this, I think you need to run "make -f client.mk configure" before building.
Attachment #690429 - Flags: review?(dmose) → review-
This fixes the android casing problem, and it also fixes the lack of <rpc/rpc.h> by simply setting NO_REG_RPC for both Android and Linux.  Presumably, the right thing to do is to split those cases apart, but maybe turning it off for Linux is ok.
Comment on attachment 691087 [details] [diff] [review]
Patch 1.v3, Fix nrappkit build

r=dmose on IRC. According to ekr the RPC code is a leftover from days past.
Attachment #691087 - Flags: review+
Attachment #690429 - Attachment is obsolete: true
Attachment #691087 - Attachment description: Patch 1.v3, WIP Fix nrappkit build → Patch 1.v3, Fix nrappkit build
Setting in-testsuite-; since this is a build fix; the compiler acts as an automated test.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/d70010fceafa
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
You need to log in before you can comment on or make changes to this bug.