Make mtransport and signalling build for iOS

RESOLVED FIXED in Firefox 41

Status

()

defect
P3
normal
Rank:
35
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Blocks 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

The changes are pretty minor, mostly just gyp/moz.build changes. Does any of this stuff need to go upstream anywhere?
Would need to see the patch.
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
bug 1171120 - Fix mtransport+signalling to build on iOS
Attachment #8614862 - Flags: review?(ekr)
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)
(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.
(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.
(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.
Can I punt that to a follow-up? I'll put the bug number in a comment in the moz.build file.
Well, my question is when the follow-up is going to get done.
(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?
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.
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.
Blocks: 1172551
(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.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/4655a2796114
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.