Closed
Bug 1171120
Opened 9 years ago
Closed 9 years ago
Make mtransport and signalling build for iOS
Categories
(Core :: WebRTC: Signaling, defect, P3)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: ted, Assigned: ted)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
40 bytes,
text/x-review-board-request
|
Details |
The changes are pretty minor, mostly just gyp/moz.build changes. Does any of this stuff need to go upstream anywhere?
Comment 1•9 years ago
|
||
Would need to see the patch.
Assignee | ||
Comment 2•9 years ago
|
||
Yeah, sorry, I'm mass-filing bugs and then pushing patches for review in batches. The changes are here: http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/rev/0b3c000ea6a4
Assignee | ||
Comment 3•9 years ago
|
||
bug 1171120 - Fix mtransport+signalling to build on iOS
Attachment #8614862 -
Flags: review?(ekr)
Comment 4•9 years ago
|
||
Comment on attachment 8614862 [details] MozReview Request: bug 1171120 - Fix mtransport+signalling to build on iOS https://reviewboard.mozilla.org/r/10099/#review8875 ::: media/mtransport/third_party/nICEr/src/util/mbslen.c:46 (Diff revision 1) > -#ifdef DARWIN > +#if defined(DARWIN) && !defined(HAVE_XLOCALE) > #define HAVE_XLOCALE > #endif Hmmm. Where is HAVE_XLOCALE being set? ::: media/webrtc/signaling/test/moz.build:7 (Diff revision 1) > -if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'uikit': > GeckoCppUnitTests([ > 'jsep_session_unittest', > 'mediaconduit_unittests', > 'mediapipeline_unittest', > 'sdp_unittests', > 'signaling_unittests', > ]) Sure woud be nice if these tests did actually run on iOS. un-possible?
Attachment #8614862 -
Flags: review?(ekr)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #4) > Comment on attachment 8614862 [details] > MozReview Request: bug 1171120 - Fix mtransport+signalling to build on iOS > > https://reviewboard.mozilla.org/r/10099/#review8875 > > ::: media/mtransport/third_party/nICEr/src/util/mbslen.c:46 > (Diff revision 1) > > -#ifdef DARWIN > > +#if defined(DARWIN) && !defined(HAVE_XLOCALE) > > #define HAVE_XLOCALE > > #endif > > Hmmm. Where is HAVE_XLOCALE being set? I don't honestly know, I think I was just silencing a warning here. This probably isn't a necessary part of the patch, just killing warnings made it easier for me to find all the build errors while porting. > > ::: media/webrtc/signaling/test/moz.build:7 > (Diff revision 1) > > -if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'uikit': > > GeckoCppUnitTests([ > > 'jsep_session_unittest', > > 'mediaconduit_unittests', > > 'mediapipeline_unittest', > > 'sdp_unittests', > > 'signaling_unittests', > > ]) > > Sure woud be nice if these tests did actually run on iOS. un-possible? I could probably spend more time to get these building, but running tests on device on iOS is hellish. The only tests I have running so far are jit tests, and that took an awful lot of effort. I'm happy to file a followup bug, I don't remember what the exact issue I was working around was.
Comment 6•9 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5) > (In reply to Eric Rescorla (:ekr) from comment #4) > > Comment on attachment 8614862 [details] > > MozReview Request: bug 1171120 - Fix mtransport+signalling to build on iOS > > > > https://reviewboard.mozilla.org/r/10099/#review8875 > > > > ::: media/mtransport/third_party/nICEr/src/util/mbslen.c:46 > > (Diff revision 1) > > > -#ifdef DARWIN > > > +#if defined(DARWIN) && !defined(HAVE_XLOCALE) > > > #define HAVE_XLOCALE > > > #endif > > > > Hmmm. Where is HAVE_XLOCALE being set? > > I don't honestly know, I think I was just silencing a warning here. This > probably isn't a necessary part of the patch, just killing warnings made it > easier for me to find all the build errors while porting. Please either figure out what's causing this or revert the change. > > > > ::: media/webrtc/signaling/test/moz.build:7 > > (Diff revision 1) > > > -if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'uikit': > > > GeckoCppUnitTests([ > > > 'jsep_session_unittest', > > > 'mediaconduit_unittests', > > > 'mediapipeline_unittest', > > > 'sdp_unittests', > > > 'signaling_unittests', > > > ]) > > > > Sure woud be nice if these tests did actually run on iOS. un-possible? > > I could probably spend more time to get these building, but running tests on > device on iOS is hellish. The only tests I have running so far are jit > tests, and that took an awful lot of effort. I'm happy to file a followup > bug, I don't remember what the exact issue I was working around was. These tests are pretty important, so I'm uncomfortable with having them just commented out. If we're going to land an iOS port, then it should have tests.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #6) > These tests are pretty important, so I'm uncomfortable with having them just > commented out. If we're going to land an iOS port, then it should have tests. Okay, but that does seem a bit of an unfair burden given that: a) they're disabled on more than one existing platform and b) I barely even have builds yet, let alone tests.
Assignee | ||
Comment 8•9 years ago
|
||
Can I punt that to a follow-up? I'll put the bug number in a comment in the moz.build file.
Comment 9•9 years ago
|
||
Well, my question is when the follow-up is going to get done.
Comment 10•9 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7) > (In reply to Eric Rescorla (:ekr) from comment #6) > > These tests are pretty important, so I'm uncomfortable with having them just > > commented out. If we're going to land an iOS port, then it should have tests. > > Okay, but that does seem a bit of an unfair burden given that: a) they're > disabled on more than one existing platform I just noticed that and it seems bad. We should fix that, right? I'd certainly be happy to have them build but not run it automation for now, if that's what you're looking for. > and b) I barely even have builds > yet, let alone tests. I'm not really following this line of argument. There are existing tests that are designed to run standalone precisely in the service of facilitating independent testing of this section of code. If you're at the point where the CPP unit tests would otherwise want to compile and link (and if not, why is this even an issue), then why can't these tests be run?
Assignee | ||
Comment 11•9 years ago
|
||
Unfortunately running tests on iOS isn't as simple as running a binary. Apple doesn't provide facilities for running a binary and gathering the output, it has to be packaged into an app bundle and signed etc, and even then there's no API for running a binary outside of a debugger. Even if I get these tests building, getting them running is likely to be a significant effort. (I spent more than a week getting jit-tests working.) Even if I get them running, I don't have any CI at all for iOS builds right now, and I don't even have a plan for CI for tests, so I'm not sure what the hurry is. I certainly understand that automated tests are important and that we should be testing what we ship, but we don't even have a plan to ship this code yet. I think filing a bug to track this work so that we can prioritize it against all the other work required to ship this code is a sane plan, and that not building these tests for iOS right now isn't going to do any harm.
Comment 12•9 years ago
|
||
After talking with brad, r=me. Please file a bug to get these tests running. This should block any exposure of WebRTC code to any real-world iOS populations.
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #6) > (In reply to Ted Mielczarek [:ted.mielczarek] from comment #5) > > (In reply to Eric Rescorla (:ekr) from comment #4) > > > Comment on attachment 8614862 [details] > > > MozReview Request: bug 1171120 - Fix mtransport+signalling to build on iOS > > > > > > https://reviewboard.mozilla.org/r/10099/#review8875 > > > > > > ::: media/mtransport/third_party/nICEr/src/util/mbslen.c:46 > > > (Diff revision 1) > > > > -#ifdef DARWIN > > > > +#if defined(DARWIN) && !defined(HAVE_XLOCALE) > > > > #define HAVE_XLOCALE > > > > #endif > > > > > > Hmmm. Where is HAVE_XLOCALE being set? > > > > I don't honestly know, I think I was just silencing a warning here. This > > probably isn't a necessary part of the patch, just killing warnings made it > > easier for me to find all the build errors while porting. > > Please either figure out what's causing this or revert the change. This turns out to be easy: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/nicer.gyp#149 The gypfile defines it on the commandline, but then mbslen.c unconditionally defines it on Darwin. That seems to be the only file that define is used in, so I can just remove the define from the gyp file instead, since the .c file takes care of it.
Updated•9 years ago
|
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4655a2796114
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4655a2796114
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•