Build webrtc signaling code using moz.build

RESOLVED FIXED in Firefox 58

Status

()

Core
WebRTC
P2
normal
Rank:
17
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

(Blocks: 1 bug)

56 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

6 months ago
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.
(Assignee)

Updated

6 months ago
Rank: 17
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'll review when I'm off PTO
Note: all of these should also be reviewed by a build peer I think

Comment 10

6 months ago
mozreview-review
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 11

6 months ago
mozreview-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 12

6 months ago
mozreview-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 13

6 months ago
mozreview-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 14

6 months ago
mozreview-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 15

6 months ago
mozreview-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 16

6 months ago
mozreview-review-reply
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
```

Comment 17

6 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

5 months ago
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.

Updated

5 months ago
Depends on: 1391716

Comment 25

5 months ago
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 27

4 months ago
mozreview-review
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 28

4 months ago
mozreview-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 29

4 months ago
mozreview-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 30

4 months ago
mozreview-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 31

4 months ago
mozreview-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 32

4 months ago
mozreview-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+
(Assignee)

Comment 33

4 months ago
mozreview-review-reply
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.

Comment 34

4 months ago
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.