1382182 - Build webrtc signaling code using moz.build

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P2)

Description

[Dan Minor [:dminor]]

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.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build jsep using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161584/

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build sdp using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161586/

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build media-conduit using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161588/

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build mediapipeline using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161590/

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build peerconnection using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161592/

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1382182 - Build signaling common using moz.build;

Review commit: https://reviewboard.mozilla.org/r/161594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161594/

Comment 7

[Dan Minor [:dminor]]

Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59dbbde5fc7668c34cfa09ab520fb0dd9ac39e9b

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

I'll review when I'm off PTO

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Note: all of these should also be reviewed by a build peer I think

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8890454 [details]
Bug 1382182 - Build signaling common using moz.build;

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

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8890450 [details]
Bug 1382182 - Build sdp using moz.build;

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

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 16

[Jan Beich]

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

[Jan Beich]

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 18

[Dan Minor [:dminor]]

Comment on attachment 8890449 [details]
Bug 1382182 - Build jsep using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161584/diff/1-2/

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8890450 [details]
Bug 1382182 - Build sdp using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161586/diff/1-2/

Comment 20

[Dan Minor [:dminor]]

Comment on attachment 8890451 [details]
Bug 1382182 - Build media-conduit using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161588/diff/1-2/

Comment 21

[Dan Minor [:dminor]]

Comment on attachment 8890452 [details]
Bug 1382182 - Build mediapipeline using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161590/diff/1-2/

Comment 22

[Dan Minor [:dminor]]

Comment on attachment 8890453 [details]
Bug 1382182 - Build peerconnection using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161592/diff/1-2/

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 8890454 [details]
Bug 1382182 - Build signaling common using moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161594/diff/1-2/

Comment 24

[Dan Minor [:dminor]]

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.

Comment 25

[Jan Beich]

Thanks. Ignoring bug 1391716 builds fine on FreeBSD. Tests from /webrtc-landing page appear to work as before.

Comment 26

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 27

[(not currently active) Ted Mielczarek]

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.

Comment 28

[(not currently active) Ted Mielczarek]

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.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 8890451 [details]
Bug 1382182 - Build media-conduit using moz.build;

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

Comment 30

[(not currently active) Ted Mielczarek]

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?

Comment 31

[(not currently active) Ted Mielczarek]

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.

Comment 32

[(not currently active) Ted Mielczarek]

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.

Comment 33

[Dan Minor [:dminor]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2b754756c4c
https://hg.mozilla.org/mozilla-central/rev/addaa13f513c
https://hg.mozilla.org/mozilla-central/rev/bf92c7059f79
https://hg.mozilla.org/mozilla-central/rev/ce7d7a06cf83
https://hg.mozilla.org/mozilla-central/rev/a3c417b41724
https://hg.mozilla.org/mozilla-central/rev/1f0a69b73cd1