Closed Bug 1382182 Opened 7 years ago Closed 7 years ago

Build webrtc signaling code using moz.build

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla58

Tracking Flags:

Tracking

Status

firefox58

---

fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Regressed 1 open bug)

Details

Attachments

(6 files)

Bug 1382182 - Build jsep using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details
Bug 1382182 - Build sdp using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details
Bug 1382182 - Build media-conduit using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details
Bug 1382182 - Build mediapipeline using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details
Bug 1382182 - Build peerconnection using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details
Bug 1382182 - Build signaling common using moz.build; 7 years ago Dan Minor [:dminor] 59 bytes, text/x-review-board-request	jesup : review+ ted : review+	Details

Dan Minor [:dminor]

Assignee

Description

•

7 years 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.

Dan Minor [:dminor]

Assignee

Updated

•

7 years ago

Rank: 17

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Dan Minor [:dminor]

Assignee

Comment 7

•

7 years ago

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

Randell Jesup [:jesup] (needinfo me)

Comment 8

•

7 years ago

I'll review when I'm off PTO

Randell Jesup [:jesup] (needinfo me)

Comment 9

•

7 years ago

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

Randell Jesup [:jesup] (needinfo me)

Comment 10

•

7 years 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+

Randell Jesup [:jesup] (needinfo me)

Comment 11

•

7 years 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+

Randell Jesup [:jesup] (needinfo me)

Comment 12

•

7 years 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+

Randell Jesup [:jesup] (needinfo me)

Comment 13

•

7 years 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+

Randell Jesup [:jesup] (needinfo me)

Comment 14

•

7 years 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+

Randell Jesup [:jesup] (needinfo me)

Comment 15

•

7 years 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+

Jan Beich

Comment 16

•

7 years 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
```

Jan Beich

Comment 17

•

7 years 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 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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Dan Minor [:dminor]

Assignee

Comment 24

•

7 years 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.

Jan Beich

Updated

•

7 years ago

Depends on: 1391716

Jan Beich

Comment 25

•

7 years ago

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

Bulk Bug Changes for mreavy's org

Comment 26

•

7 years ago

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

Priority: P1 → P2

(not currently active) Ted Mielczarek

Comment 27

•

7 years 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+

(not currently active) Ted Mielczarek

Comment 28

•

7 years 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+

(not currently active) Ted Mielczarek

Comment 29

•

7 years 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+

(not currently active) Ted Mielczarek

Comment 30

•

7 years 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+

(not currently active) Ted Mielczarek

Comment 31

•

7 years 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+

(not currently active) Ted Mielczarek

Comment 32

•

7 years 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+

(not currently active) Ted Mielczarek

Updated

•

7 years ago

Blocks: wsl-build

Dan Minor [:dminor]

Assignee

Comment 33

•

7 years 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.

Pulsebot

Comment 34

•

7 years 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

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

Comment 35

•

7 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox58: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Andreas Pehrson [:pehrsons]

Updated

•

4 years ago

Regressions: 1665559

You need to log in before you can comment on or make changes to this bug.