Closed Bug 1382182 Opened 2 years ago Closed 2 years ago

Build webrtc signaling code using moz.build

Categories

(Core :: WebRTC, enhancement, P2)

56 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(6 files)

We currently use gyp to build the webrtc signaling code (media/webrtc/signaling/*).

With the work in Bug 1336429 to switch from gyp to gn to build webrtc.org code, we should try to find a way of building the signaling code using moz.build. It would be ugly to continue to use gyp here when webrtc.org has switched to gn, and it would be unfortunate to have to write our own gn files for this.
Rank: 17
I'll review when I'm off PTO
Note: all of these should also be reviewed by a build peer I think
Comment on attachment 8890453 [details]
Bug 1382182 - Build peerconnection using moz.build;

https://reviewboard.mozilla.org/r/161592/#review168402

::: media/webrtc/signaling/src/peerconnection/moz.build:21
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'Linux':
> +  DEFINES['WEBRTC_LINUX'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +elif CONFIG['OS_TARGET'] == 'Darwin':
> +  DEFINES['WEBRTC_MAC'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +  DEFINES['WEBRTC_WIN'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +  DEFINES['HAVE_WINSOCK2_H'] = True
> +else:
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True

I think there are a couple of moz.builds that need this stuff (because they include webrtc stuff, or build it), and if things are added often one is fogotten.

perhaps these could be moved into some included file everything shares?  dom/media, dom/media/webrtc, dom/media/systemservices, peerconnection (and other signaling/src/* dirs in the future), etc.
Attachment #8890453 - Flags: review?(rjesup) → review+
Comment on attachment 8890454 [details]
Bug 1382182 - Build signaling common using moz.build;

https://reviewboard.mozilla.org/r/161594/#review168404
Attachment #8890454 - Flags: review?(rjesup) → review+
Comment on attachment 8890449 [details]
Bug 1382182 - Build jsep using moz.build;

https://reviewboard.mozilla.org/r/161584/#review168408

::: media/webrtc/signaling/src/jsep/moz.build:12
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'Linux':
> +  DEFINES['WEBRTC_LINUX'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +elif CONFIG['OS_TARGET'] == 'Darwin':
> +  DEFINES['WEBRTC_MAC'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +  DEFINES['WEBRTC_WIN'] = True
> +

See comment on other patch -- share these somehow
Attachment #8890449 - Flags: review?(rjesup) → review+
Comment on attachment 8890450 [details]
Bug 1382182 - Build sdp using moz.build;

https://reviewboard.mozilla.org/r/161586/#review168412
Attachment #8890450 - Flags: review?(rjesup) → review+
Comment on attachment 8890451 [details]
Bug 1382182 - Build media-conduit using moz.build;

https://reviewboard.mozilla.org/r/161588/#review168414

::: media/webrtc/signaling/src/media-conduit/moz.build:19
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'Linux':
> +  DEFINES['WEBRTC_LINUX'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +elif CONFIG['OS_TARGET'] == 'Darwin':
> +  DEFINES['WEBRTC_MAC'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +  DEFINES['WEBRTC_WIN'] = True
> +else:
> +  DEFINES['WEBRTC_POSIX'] = True

and again
Attachment #8890451 - Flags: review?(rjesup) → review+
Comment on attachment 8890452 [details]
Bug 1382182 - Build mediapipeline using moz.build;

https://reviewboard.mozilla.org/r/161590/#review168416

::: media/webrtc/signaling/src/mediapipeline/moz.build:17
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'Linux':
> +  DEFINES['WEBRTC_LINUX'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +elif CONFIG['OS_TARGET'] == 'Darwin':
> +  DEFINES['WEBRTC_MAC'] = True
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +  DEFINES['WEBRTC_WIN'] = True
> +  DEFINES['HAVE_UINT64_T'] = True
> +  DEFINES['HAVE_WINSOCK2_H'] = True
> +else:
> +  DEFINES['WEBRTC_POSIX'] = True
> +  DEFINES['HAVE_UINT64_T'] = True

and one more time
Attachment #8890452 - Flags: review?(rjesup) → review+
Comment on attachment 8890452 [details]
Bug 1382182 - Build mediapipeline using moz.build;

https://reviewboard.mozilla.org/r/161590/#review168416

> and one more time

This can also be deduplicated e.g.,
```python
DEFINES['HAVE_UINT64_T'] = True
if CONFIG['OS_TARGET'] != 'WINNT':
  DEFINES['WEBRTC_POSIX'] = True

if CONFIG['OS_TARGET'] == 'Linux':
  DEFINES['WEBRTC_LINUX'] = True
elif CONFIG['OS_TARGET'] == 'Darwin':
  DEFINES['WEBRTC_MAC'] = True
elif CONFIG['OS_TARGET'] == 'WINNT':
  DEFINES['WEBRTC_WIN'] = True
  DEFINES['HAVE_WINSOCK2_H'] = True
```
When combining common WEBRTC_<PLATFORM> also include existing usage e.g.,

$ rg -g moz.build WEBRTC_LINUX
media/webrtc/signaling/gtest/moz.build
10:        DEFINES['WEBRTC_LINUX'] = True

media/webrtc/trunk/gtest/moz.build
59:        DEFINES['WEBRTC_LINUX'] = True

http://searchfox.org/mozilla-central/search?q=WEBRTC_LINUX&path=moz.build
As suggested, I've added a webrtc.mozbuild file that can be included to set the defines and updated the existing uses in tree to make use of it.
Depends on: 1391716
Thanks. Ignoring bug 1391716 builds fine on FreeBSD. Tests from /webrtc-landing page appear to work as before.
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Comment on attachment 8890449 [details]
Bug 1382182 - Build jsep using moz.build;

https://reviewboard.mozilla.org/r/161584/#review187160

::: dom/media/systemservices/moz.build:28
(Diff revision 2)
> -if CONFIG['OS_TARGET'] == 'WINNT':
> +if CONFIG['OS_TARGET'] != 'WINNT':
> -    DEFINES['WEBRTC_WIN'] = True
> -else:
> -    DEFINES['WEBRTC_POSIX'] = True
>      # Must match build/gyp.mozbuild: enable_libevent
>      DEFINES['WEBRTC_BUILD_LIBEVENT'] = True

Does this want to move to webrtc.mozbuild as well?

::: media/webrtc/signaling/gtest/moz.build:6
(Diff revision 2)
>  # -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +include('../../webrtc.mozbuild')

nit: I think the topsrcdir-absolute path is more readable here.

::: media/webrtc/signaling/moz.build:7
(Diff revision 2)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +DIRS += [
> +    'src/jsep',

You could skip having this file if nothing is actually going to get built here and just list the subdirs directly in media/webrtc/moz.build.

::: media/webrtc/signaling/src/jsep/moz.build:6
(Diff revision 2)
> +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +include('../../../webrtc.mozbuild')

again re: the topsrcdir path.
Attachment #8890449 - Flags: review?(ted) → review+
Comment on attachment 8890450 [details]
Bug 1382182 - Build sdp using moz.build;

https://reviewboard.mozilla.org/r/161586/#review187162

::: media/webrtc/signaling/src/sdp/sipcc/moz.build:5
(Diff revision 2)
> +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Can you just merge this moz.build file into the parent one? This code all gets linked into the same place, splitting it into separate moz.build files doesn't really buy you much.
Attachment #8890450 - Flags: review?(ted) → review+
Comment on attachment 8890451 [details]
Bug 1382182 - Build media-conduit using moz.build;

https://reviewboard.mozilla.org/r/161588/#review187166
Attachment #8890451 - Flags: review?(ted) → review+
Comment on attachment 8890452 [details]
Bug 1382182 - Build mediapipeline using moz.build;

https://reviewboard.mozilla.org/r/161590/#review187168

::: media/webrtc/signaling/src/mediapipeline/moz.build:30
(Diff revision 2)
> +]
> +
> +FINAL_LIBRARY = 'xul'
> +
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-Wno-shadow']

Maybe this wants to wind up in webrtc.mozbuild?
Attachment #8890452 - Flags: review?(ted) → review+
Comment on attachment 8890453 [details]
Bug 1382182 - Build peerconnection using moz.build;

https://reviewboard.mozilla.org/r/161592/#review187170

::: media/webrtc/signaling/src/peerconnection/moz.build:23
(Diff revision 2)
> +    '/media/webrtc/signaling/src/media-conduit',
> +    '/media/webrtc/signaling/src/mediapipeline',
> +    '/media/webrtc/trunk',
> +]
> +
> +# Multiple uses of logTag

How hard would it be to fix the `logTag` issue? Being able to use `UNIFIED_SOURCES` would speed up builds a bit.
Attachment #8890453 - Flags: review?(ted) → review+
Comment on attachment 8890454 [details]
Bug 1382182 - Build signaling common using moz.build;

https://reviewboard.mozilla.org/r/161594/#review187172

::: media/webrtc/signaling/src/common/moz.build:8
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +DIRS += [
> +    'browser_logging',
> +    'time_profiling',

I would just merge the contents of both of the other moz.build files here into this one.
Attachment #8890454 - Flags: review?(ted) → review+
Comment on attachment 8890453 [details]
Bug 1382182 - Build peerconnection using moz.build;

https://reviewboard.mozilla.org/r/161592/#review187170

> How hard would it be to fix the `logTag` issue? Being able to use `UNIFIED_SOURCES` would speed up builds a bit.

There are few other places with similar problems. I filed Bug 1402334 to follow up on this.
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b754756c4c
Build jsep using moz.build; r=ted,jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/addaa13f513c
Build sdp using moz.build; r=ted,jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf92c7059f79
Build media-conduit using moz.build; r=ted,jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7d7a06cf83
Build mediapipeline using moz.build; r=ted,jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c417b41724
Build peerconnection using moz.build; r=ted,jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0a69b73cd1
Build signaling common using moz.build; r=ted,jesup
You need to log in before you can comment on or make changes to this bug.