Build webrtc.org code using 'gn'

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
Rank:
15
RESOLVED FIXED
2 years ago
Last year

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

2 years ago
Once 'gn' support is available, we can use it build the webrtc.org code.
Assignee

Updated

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

Comment 3

2 years 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)
Assignee

Comment 18

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

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

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

2 years ago
Attachment #8922348 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8925620 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8925621 - Attachment is obsolete: true
Assignee

Comment 25

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

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

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

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

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

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

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

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

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

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

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

Last year
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.)
Flags: needinfo?(dminor)
Assignee

Comment 47

Last year
(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
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.
No longer blocks: 1399679
Assignee

Updated

Last year
Blocks: 1430779
Assignee

Comment 51

Last year
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(dminor)
Resolution: FIXED → ---
Assignee

Comment 58

Last year
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)
See Also: → 1432761
Assignee

Updated

Last year
See Also: 1432761
Assignee

Comment 60

Last year
(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)
Depends on: 1434589

Updated

Last year
Flags: needinfo?(martin)
Depends on: 1437670
Assignee

Updated

Last year
Depends on: 1438554
Assignee

Comment 63

Last year
BSD discussion has moved to Bug 1437670.
Flags: needinfo?(jbeich)

Updated

Last year
Depends on: 1437345
This undid the changes from bug 1257888, and broke building on at least powerpc64el as a consequence.
Depends on: 1462873
You need to log in before you can comment on or make changes to this bug.