Build webrtc.org code using 'gn'

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
Rank:
15
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

57 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
Once 'gn' support is available, we can use it build the webrtc.org code.
(Assignee)

Updated

a year ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 15
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
Comment on attachment 8904968 [details]
Bug 1393119 - Add webrtc.org trunk/build/ files to support gn build;

https://reviewboard.mozilla.org/r/176798/#review182484

::: media/webrtc/trunk/build/config/sysroot.gni:18
(Diff revision 1)
> +  use_sysroot = current_cpu != "s390x" && current_cpu != "s390" &&
> +                current_cpu != "ppc64" && current_cpu != "ppc"

Looks like we need to set this to false at some point so we don't get tripped up here on eg x86.
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 18

11 months ago
mozreview-review
Comment on attachment 8933021 [details]
Bug 1393119 - Add webrtc gn build config.

https://reviewboard.mozilla.org/r/204012/#review210756

::: media/webrtc/moz.build:76
(Diff revision 1)
> -GYP_DIRS['trunk'].variables = gyp_vars_copy
> +
> +gn_vars_copy = gn_vars.copy()
> +
> +GN_DIRS['trunk'].variables = gn_vars_copy
> +GN_DIRS['trunk'].mozilla_flags = [
> +    "-fobjc-arc"

we also need '-mfpu=neon'
(Assignee)

Comment 19

11 months ago
mozreview-review
Comment on attachment 8933021 [details]
Bug 1393119 - Add webrtc gn build config.

https://reviewboard.mozilla.org/r/204012/#review211104

::: build/gn.mozbuild:32
(Diff revision 1)
> +}
> +gn_vars['target_os'] = flavors.get(os)
> +
> +arches = {
> +    'x86_64': 'x64',
> +    'x86': 'ia32',

This should be x86 not ia32.
(Assignee)

Comment 20

10 months ago
mozreview-review
Comment on attachment 8933021 [details]
Bug 1393119 - Add webrtc gn build config.

https://reviewboard.mozilla.org/r/204012/#review213522

Looks good to me, thank you.

::: media/webrtc/trunk/gtest/moz.build:41
(Diff revision 1)
>      'speex',
>      'webrtc',
> -    'webrtc_common',
> -    'webrtc_i420',
> -    'webrtc_lib',
> -    'webrtc_utility',
> +    'webrtc_common_gn',
> +    'webrtc_gn',
> +    'webrtc_i420_gn',
> +    # 'webrtc_utility',

Please remove this entirely.

::: media/webrtc/trunk/webrtc/BUILD.gn:247
(Diff revision 1)
>  
>      defines = []
>  
>      deps = [
>        ":webrtc_common",
> -      "api",
> +#      "api",

Please revert this change, I fix this up in a later commit.
Attachment #8933021 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8922348 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8925620 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8925621 - Attachment is obsolete: true
(Assignee)

Comment 25

10 months ago
Hi Jan,

We're in the process of moving our webrtc.org builds from gyp to gn. Since we don't want to run 'gn' as part of our build, we're using gn to generate json files and then generating moz.build files from those json files. The json files I've used to generate the moz.build in the patch here were generated from [1].

For FreeBSD I think there are two possible ways ahead. One would be to get 'gn' running on FreeBSD and generate json files. You would likely have to patch some of the gn files for this to work, but this has the advantage that we could attempt to get those changes landed on upstream webrtc.org. I have no idea how much effort is required to get gn running on FreeBSD, perhaps it is already supported.

The other possibility would be to copy the json output from a Linux build and modify it to have the proper defines and system libraries for FreeBSD.

You can find some instructions on running gn at Bug 1336429#c42. In brief, you would want to grab the json files from [1], copy them to media/webrtc/gn-configs and add the following lines to your mozconfig:
#ac_add_options --build-backends=GnConfigGen
#ac_add_options --build-backends=GnMozbuildWriter

Uncommenting the first line and running ./mach configure will attempt to run gn and generate a json file for your current configuration. Uncommenting the second line and running ./mach configure will generate moz.build files for all configurations from the json files under media/webrtc/gn-configs.

I don't plan on landing these changes until the first week of January at the earliest.

[1] https://hg.mozilla.org/users/dminor_mozilla.com/webrtc.org_gn-configs/
Flags: needinfo?(jbeich)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

10 months ago
mozreview-review
Comment on attachment 8904969 [details]
Bug 1393119 - Update gn files for mozilla build;

https://reviewboard.mozilla.org/r/176800/#review214186

::: media/webrtc/trunk/build/config/BUILD.gn:52
(Diff revision 6)
> -  defines = [ "V8_DEPRECATION_WARNINGS" ]
> +  defines = []
> +  if (!build_with_mozilla) {
> +    defines += [ "V8_DEPRECATION_WARNINGS" ]
> +  }
> +

is this actually needed?  Does V8_DEPRECATION_WARNINGS cause anything to happen when we build?  (I presume so, just want to understand why we need to carry this change)

::: media/webrtc/trunk/build/mac/find_sdk.py:89
(Diff revision 6)
> +  else:
> +    # Mozilla builds cross-compile on Linux, so return some fake data to keep
> +    # the build system happy.
> +    print "."
> +    print "10.9"
>    sys.exit(0)

I presume this needs to be linked to the Mac SDK version we're using - can we make that more explicit, and also add a change somewhere else we know will get touched as part of an SDK update to remind people to update this?  Or (maybe) pull the version from some shared spot, or have it be passed into the GN code?

::: media/webrtc/trunk/webrtc/BUILD.gn:31
(Diff revision 6)
> +       "WEBRTC_VOE_EXTERNAL_REC_AND_PLAYOUT",
> +    ]

Check if this is needed with padenot's recent landing

::: media/webrtc/trunk/webrtc/api/BUILD.gn:16
(Diff revision 6)
> -  public_deps = [
> +  public_deps = []
> +
> +  if (!build_with_mozilla) {
> +    deps += [
> -    ":libjingle_peerconnection",
> +      ":libjingle_peerconnection",
> -  ]
> +    ]
> -}
> +  }
> +}

Shouldn't this be public_deps += ?

::: media/webrtc/trunk/webrtc/build/webrtc.gni:33
(Diff revision 6)
>    # Used to specify an external OpenSSL include path when not compiling the
>    # library that comes with WebRTC (i.e. rtc_build_ssl == 0).
> -  rtc_ssl_root = ""
> +  rtc_ssl_root = "dummy rtc ssl root"

Not sure why this is needed, but I assume it is
Attachment #8904969 - Flags: review?(rjesup)

Comment 29

10 months ago
mozreview-review
Comment on attachment 8904968 [details]
Bug 1393119 - Add webrtc.org trunk/build/ files to support gn build;

https://reviewboard.mozilla.org/r/176798/#review214208

This is actually rs=me for an import; I didn't vet the entire upstream codebase.  This needs a build peer review obviously

::: media/webrtc/trunk/build/config/BUILDCONFIG.gn:141
(Diff revision 6)
> +      (current_os == "linux" && current_cpu != "s390x" &&
> +       current_cpu != "s390" && current_cpu != "ppc64" && current_cpu != "ppc")

Amusing, IBM mainframe support
Attachment #8904968 - Flags: review?(rjesup) → review+

Comment 30

10 months ago
mozreview-review
Comment on attachment 8937525 [details]
Bug 1393119 - Add generated files;

https://reviewboard.mozilla.org/r/208212/#review214212

rs=me - suggest a build peer as well
Attachment #8937525 - Flags: review?(rjesup) → review+

Comment 31

10 months ago
mozreview-review
Comment on attachment 8937704 [details]
Bug 1393119 - Remove webrtc gyp files;

https://reviewboard.mozilla.org/r/208424/#review214214

rs=me
Attachment #8937704 - Flags: review?(rjesup) → review+
(Assignee)

Comment 32

10 months ago
mozreview-review-reply
Comment on attachment 8904969 [details]
Bug 1393119 - Update gn files for mozilla build;

https://reviewboard.mozilla.org/r/176800/#review214186

> I presume this needs to be linked to the Mac SDK version we're using - can we make that more explicit, and also add a change somewhere else we know will get touched as part of an SDK update to remind people to update this?  Or (maybe) pull the version from some shared spot, or have it be passed into the GN code?

These values actually don't matter at all. I'll update the comment and the values to make it clearer that they are not used.

Comment 33

10 months ago
Landry, Martin, do you have gn packaged on OpenBSD and NetBSD? See comment 25.
Flags: needinfo?(martin)
Flags: needinfo?(landry)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Nope, and i have to admit i'm tired of the constant changes in the build systems that i stopped following / adapt to them. Seems gn has no actual releases, and has to be extracted from chrome source ? how convenient..
Flags: needinfo?(landry)
(Assignee)

Comment 38

10 months ago
Just to be clear, gn is not required to build firefox, just to code generate moz.build files for use by the normal build system, so only the package maintainer needs to have gn available. I realize that this is not a great situation, but we're stuck with it as upstream dropped support for gyp over a year ago. Sorry :/.

Comment 39

10 months ago
mozreview-review
Comment on attachment 8904969 [details]
Bug 1393119 - Update gn files for mozilla build;

https://reviewboard.mozilla.org/r/176800/#review214550

::: media/webrtc/trunk/build/mac/find_sdk.py:92
(Diff revisions 6 - 7)
> +    print "."
>      print "."

I presume something is looking for '.'; if so 1.2 (or something of that nature) might be less likely to break in the future -- but likely it doesn't matter.

::: media/webrtc/trunk/webrtc/BUILD.gn:29
(Diff revisions 6 - 7)
> -    defines += [
> +    defines += [ "WEBRTC_MOZILLA_BUILD" ]
> -       "WEBRTC_MOZILLA_BUILD",
> -       "WEBRTC_VOE_EXTERNAL_REC_AND_PLAYOUT",
> -    ]

I take it this was unneeded cruft?
Attachment #8904969 - Flags: review?(rjesup) → review+
Comment hidden (mozreview-request)

Comment 41

10 months ago
(In reply to Dan Minor [:dminor] from comment #25)
> #ac_add_options --build-backends=GnConfigGen
[...]
> Uncommenting the first line and running ./mach configure will attempt to run gn
> and generate a json file for your current configuration.

1. Installed http://freshports.org/devel/chromium-gn
2. Uncomented the first line (nothing else in .mozconfig)
3. Ran ./mach configure
4. Cannot find generated json files from WebRTC

$ find . -name gn-\*
./python/mozbuild/mozbuild/test/backend/data/gn-processor
./python/mozbuild/mozbuild/test/backend/data/gn-processor/gn-configs
./browser/config/tooltool-manifests/win32/gn-build.manifest
$ find . -name \*Gn\*
./toolkit/system/gnome/nsGnomeModule.cpp
./obj-x86_64-unknown-freebsd11.1/backend.GnConfigGenBackend.in
./obj-x86_64-unknown-freebsd11.1/backend.GnConfigGenBackend


Also, which moz.build files are generated by GnMozbuildWriter? I'm curious how those handle --disable-pulseaudio --with-system-libvpx --with-system-libevent.
(Assignee)

Comment 42

9 months ago
According to the documentation at Bug 1427840, it looks like I told you the old way of running the moz.build generation. Please try with the updated instructions. If you're still not having any luck, please attach the output of running the mach command and needinfo chmanchester.

Updated

9 months ago
Blocks: 1429819
Considering the timing wrt the beta merge, and the fact that I just discovered this changed at least one configuration item, I'm inclined to say this should be backed out until we're sure it actually doesn't change anything. The one thing that I found is that it enables the alsa backend in audio_device. Who knows what else sneaked in.
Blocks: 1399679
(Assignee)

Updated

9 months ago
Depends on: 1430094
(In reply to Mike Hommey [:glandium] from comment #45)
> Considering the timing wrt the beta merge, and the fact that I just
> discovered this changed at least one configuration item, I'm inclined to say
> this should be backed out until we're sure it actually doesn't change
> anything. The one thing that I found is that it enables the alsa backend in
> audio_device. Who knows what else sneaked in.

Dan, does the above sound ok?  (We're building 59.0b1 today, and will keep syncing central to beta until the 22nd when central bumps to 60.)
status-firefox57: affected → ---
Flags: needinfo?(dminor)
(Assignee)

Comment 47

9 months ago
(In reply to Julien Cristau [:jcristau] from comment #46)
> (In reply to Mike Hommey [:glandium] from comment #45)
> > Considering the timing wrt the beta merge, and the fact that I just
> > discovered this changed at least one configuration item, I'm inclined to say
> > this should be backed out until we're sure it actually doesn't change
> > anything. The one thing that I found is that it enables the alsa backend in
> > audio_device. Who knows what else sneaked in.
> 
> Dan, does the above sound ok?  (We're building 59.0b1 today, and will keep
> syncing central to beta until the 22nd when central bumps to 60.)

That sounds fine to me. The problems found so far are easy enough to fix, but glandium makes a good point that we don't know what else might be found. I don't think there's a strong reason to have this on 59 rather than 60.
Flags: needinfo?(dminor)
Backed out.
https://hg.mozilla.org/mozilla-central/rev/b38027ccdd67154b58fdb7736a6a2b932a1d3cc0
Status: RESOLVED → REOPENED
status-firefox59: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---
Status: REOPENED → ASSIGNED
When this relands, it would be good if Bug 1429819 can be rolled into it.
FYI. We plan to include this backout in the build for 59.0b1.

Updated

9 months ago
No longer blocks: 1399679
(Assignee)

Updated

9 months ago
Blocks: 1430779
(Assignee)

Comment 51

9 months ago
Beyond building the ALSA audio device backend, this also flipped audio processing debugging off, disabled the ISAC codec, unset MULTI_MONITOR_SUPPORT, and removed the -msse2 flag. Fixes for all of these are now rolled in.

Updated

9 months ago
Status: RESOLVED → REOPENED
Flags: needinfo?(dminor)
Resolution: FIXED → ---
(Assignee)

Comment 58

9 months ago
It appears the reland of the patch was from the original attachments, not from the version pushed in Comment 52, which had the fixes to disable ALSA among other things. I'll do another try run just to be safe.
Flags: needinfo?(dminor)
Please also add this change again: https://hg.mozilla.org/mozilla-central/rev/45f79eef5da2

It had to be reverted during the merge to inbound.
Flags: needinfo?(dminor)

Updated

9 months ago
See Also: → bug 1432761
(Assignee)

Updated

9 months ago
See Also: bug 1432761
(Assignee)

Comment 60

9 months ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #59)
> Please also add this change again:
> https://hg.mozilla.org/mozilla-central/rev/45f79eef5da2
> 
> It had to be reverted during the merge to inbound.

Will do.
Flags: needinfo?(dminor)

Updated

9 months ago
Depends on: 1434589

Updated

8 months ago
Flags: needinfo?(martin)

Updated

8 months ago
Depends on: 1437670
(Assignee)

Updated

8 months ago
Depends on: 1438554
(Assignee)

Comment 63

8 months ago
BSD discussion has moved to Bug 1437670.
Flags: needinfo?(jbeich)

Updated

7 months ago
Depends on: 1437345
This undid the changes from bug 1257888, and broke building on at least powerpc64el as a consequence.

Updated

5 months ago
Depends on: 1462873
You need to log in before you can comment on or make changes to this bug.