Closed Bug 1393119 Opened 7 years ago Closed 7 years ago

Build webrtc.org code using 'gn'

Categories

(Core :: WebRTC, enhancement, P2)

Product:
Core
Component:
WebRTC
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Depends on 2 open bugs)

Details

Attachments

(6 files, 3 obsolete files)

Bug 1393119 - Add webrtc.org trunk/build/ files to support gn build;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1393119 - Update gn files for mozilla build;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1393119 - Add webrtc gn build config.
59 bytes, text/x-review-board-request
Details
Bug 1393119 - Add generated files;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1393119 - Add webrtc gn build config.
59 bytes, text/x-review-board-request
Details
Bug 1393119 - Remove webrtc gyp files;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Once 'gn' support is available, we can use it build the webrtc.org code.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 15
Priority: -- → P1
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 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'
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.
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+
Attachment #8922348 - Attachment is obsolete: true
Attachment #8925620 - Attachment is obsolete: true
Attachment #8925621 - Attachment is obsolete: true
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 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 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 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 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+
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.
Landry, Martin, do you have gn packaged on OpenBSD and NetBSD? See comment 25.
Flags: needinfo?(martin)
Flags: needinfo?(landry)
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)
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 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+
(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.
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.
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
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)
(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.
No longer blocks: 1399679
Blocks: 1430779
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 → ---
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
See Also: 1432761 →
(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
Flags: needinfo?(martin)
Depends on: 1437670
Depends on: 1438554
BSD discussion has moved to Bug 1437670.
Flags: needinfo?(jbeich)
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.

Attachment

General

Creator:
Created:
Updated:
Size: