Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Build Makefiles from WebRTC GYP files

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jesup, Assigned: ted)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 20 obsolete attachments)

10.62 KB, patch
Details | Diff | Splinter Review
486 bytes, patch
ted
: review+
khuey
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Probably one Makefile for each section
Ideally we could generate these from the upstream .gyp files directly, to make it easier to track changes from upstream. I have no idea how difficult that would be.
There are also Android.mk files scattered throughout the code. I suspect they're more likely to have version skew than the .gyp files, but they might be easier to scan for lists of files.
(more thinking aloud)

Looking at webrtc/trunk/DEPS, we already have most of the dependencies: libvpx, libjpeg(turbo), yasm, expat; or they're required for the gyp build (like gyp itself) and the testing framework. 

That leaves libprotobuf, jsoncpp, and the patched libjingle as remaining external dependencies, which is a small enough number it may be better to work with the bare webrtc svn repository, which simplilfies things.
What is libprotobuf actually being used for?
Looks like the audio_processing module is using it for debug tracing.
(Reporter)

Comment 6

6 years ago
Right - and nothing else.  Very tempting to find a way to remove it. I'm tempted for the moment to import without it and to turn off those tests for now.

That leaves jsoncpp and libjingle (which includes things like ICE for some reason.
(Reporter)

Comment 10

6 years ago
Discussion of repo plans moved to bug 688847.  Let this one return to makefiles...  I'll hide the comments I made here to avoid confusion
Certainly we should update the bug here with links to google's rietveld instance when the upstream status for a patch changes. I'd suggest, therefore, that our webrtc patch bugs not be closed until they're accepted or rejected upstream.
Created attachment 562913 [details] [diff] [review]
work in progress

This is just a snapshot, showing how things could look. This is based on (manually) copying the LOCAL_MODULE, LOCAL_SRC_FILES, and LOCAL_C_INCLUDES from media/webrtc/src/common_audio/resampler/main/source/Android.mk

I moved webrtc/trunk/Makefile.in up a level because I'd hoped to flatten the whole thing, more like the libvpx build works. However, there are so many submodules I think it doesn't actually make sense to do that.
Re android.mk vs *.gyp; in theory the gyp files are easier to parse, being json-ish, but there are a lot of cross includes and the build stuff isn't well modularized, so the Android.mk files looked like a cleaner place to start.
Created attachment 563245 [details] [diff] [review]
work in progress v2

The patch includes a rough python script which walks the tree, parses the source and include variables from Android.mk files and generates corresponding Makefile.in files, which are also included in the patch.

Still TODO:

* add DIRS support to write_makefile so the script can update the top-level webrtc/Makefile.in (the current version is manual)
* s/CPPSRCS/CSRCS/ for .c source files
* add some missing module-wide includes, defines, and other flags
* filter out test directories
Attachment #562913 - Attachment is obsolete: true
Comment on attachment 563245 [details] [diff] [review]
work in progress v2

Didn't get this finished today, but updating with work in progress.
Created attachment 563269 [details] [diff] [review]
updated WIP

I made some update to the python script:
* add DIRS support to write_makefile so the script can update the top-level webrtc/Makefile.in
* support for CPPSRCS and CSRCS
* filter out test directories

This still fails to compile probably because we're missing some include.
Attachment #563245 - Attachment is obsolete: true
(Reporter)

Comment 17

6 years ago
Created attachment 563346 [details]
More updated file (WIP)

Note: this is just update_makefiles.py.   Gets pretty far, dies on dependency on protobuf includes it appears.  Evil hacks to even get it this far; the model is a tough one for complex makefiles; I punted on the file with 3 effective makefiles - one main and two tests - and only did the first part.
Created attachment 563592 [details] [diff] [review]
updated WIP

This builds some parts of the tree, but I still can't link code using it.
Attachment #563269 - Attachment is obsolete: true
Created attachment 563598 [details] [diff] [review]
current work in progress

I spent some time merging Randell and Fabrice's changes this afternoon. Just a snapshot of my tree, still based on parsing the Android.mk files. Didn't get any further than Randell last night.
Created attachment 563622 [details] [diff] [review]
updated WIP

Now creating a shared_libs.mk included from layout/build/Makefile.in and popuplated with the libraries we build.

This builds, but I still can't link code from netwerk/protocol/device with webrtc.
Attachment #563592 - Attachment is obsolete: true
(Reporter)

Comment 21

6 years ago
Created attachment 564372 [details] [diff] [review]
Add makefiles that invoke the native webrtc build system - WIP
(Reporter)

Comment 22

6 years ago
Created attachment 564470 [details] [diff] [review]
patch to use gyp directly and put object files in OBJDIR - WIP
(Reporter)

Updated

6 years ago
Attachment #564372 - Attachment is obsolete: true
(Reporter)

Comment 23

6 years ago
Created attachment 564483 [details] [diff] [review]
Patch that gets to the link phase and fails - WIP

Gets to actual link errors for libxul
Attachment #564470 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Depends on: 643692
(Reporter)

Comment 24

6 years ago
Created attachment 564763 [details] [diff] [review]
build with webrtc - works, all-in-one
(Reporter)

Updated

6 years ago
Attachment #563346 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #563598 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #563622 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #564483 - Attachment is obsolete: true
(Reporter)

Comment 25

6 years ago
committed:
https://hg.mozilla.org/projects/alder/rev/7b88b3cb72f2

Build with this in .mozconfig:

ac_add_options --enable-webrtc
ac_add_options --with-system-libvpx
ac_add_options --disable-crashreporter
ac_add_options --enable-webrtc is useless, since it's enabled by default.
You can disable it with --disable-webrtc
to build with the current alder repository, you also need to export OBJDIR.
Created attachment 564904 [details] [diff] [review]
followup

Adding more header files to EXPORTS
(Reporter)

Comment 29

6 years ago
Created attachment 567607 [details] [diff] [review]
patches to make Mac build
(Reporter)

Comment 30

6 years ago
Note: fabrice's patches for camera access (on another bug) fail to build for mac
(Assignee)

Comment 31

6 years ago
Created attachment 583771 [details] [diff] [review]
generate Makefile.in build files for WebRTC from GYP files

Here's the current output of my GYP->Makefile.in generator from bug 643692. It doesn't actually build properly yet, but you can see what the output is likely to look like.
(Reporter)

Comment 32

6 years ago
Looks good.  I assume we'll run the gyp-to-Makefile.in process when we import a new set of code from google (we'd add a command to do it to media/webrtc/webrtc-update.sh), or manually run it if we change a .gyp file.

Thanks Ted!
(Assignee)

Comment 33

6 years ago
Yeah, running it manually (or as part of a script) on import/gyp change was my intention.
(Assignee)

Comment 34

6 years ago
Created attachment 584793 [details] [diff] [review]
generate Makefile.in build files for WebRTC from GYP files

This set of Makefiles builds successfully for me on Linux. I don't know if the output is correct, but it's a start.
(Assignee)

Updated

6 years ago
Attachment #583771 - Attachment is obsolete: true
(Assignee)

Comment 35

6 years ago
Created attachment 584794 [details] [diff] [review]
tweak include dir for webrtc vp8 build

This patch is also necessary to make the build succeed. It just tweaks an include path for the vp8 build.
(Assignee)

Comment 36

6 years ago
Comment on attachment 584793 [details] [diff] [review]
generate Makefile.in build files for WebRTC from GYP files

These Makefiles work on Linux, but due to the way GYP functions, I don't think we'll be able to go with this exact approach. We'll probably have to generate the Makefiles from the GYP files at configure-time so that the per-platform bits are correct. The other alternative that derf suggested is to generate three sets of Makefiles (one per-platform), and select the correct platform to be included from a common Makefile at build time. That's possible, but it might require generating each platform's Makefiles on that specific platform, which would make updating the WebRTC snapshot annoying.
Attachment #584793 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #584794 - Attachment is obsolete: true
(Assignee)

Comment 37

6 years ago
Created attachment 584994 [details] [diff] [review]
minor WebRTC GYP tweaks

Just accumulating another minor tweak to the GYP files.

With these tweaks + the GYP backend from the other bug, I was able to generate Makefiles and build WebRTC successfully on a Mac build. Now on to Windows...
(Assignee)

Comment 38

6 years ago
these makefiles were generated using the following command:
cd media/webrtc/trunk; ./build/gyp_chromium --debug=all --format=mozmake --depth=. -D build_with_mozilla=1 -D enable_protobuf=0 -G relative_srcdir=media/webrtc/trunk webrtc.gyp
(Assignee)

Comment 39

6 years ago
Created attachment 585507 [details] [diff] [review]
generate Makefile.in build files for WebRTC from GYP files

Just a checkpoint.
(Assignee)

Comment 40

6 years ago
Created attachment 585508 [details] [diff] [review]
workaround visibility problem with system expat

This patch works around visibility issues with the system expat that libjingle links to by adding a system-headers-style wrapper header.

In the long run, I'm not sure we want to link to system expat, but we can worry about that later. (Note that libxul is currently transitively linked to expat on my Linux system anyway.)
(Assignee)

Comment 41

6 years ago
Created attachment 585509 [details] [diff] [review]
Fix webrtc shared_libs.mk to use the paths from the new Makefiles

This patch (which is actually from jesup) fixes the paths in shared_libs.mk to match the locations where things get built with the new Makefiles.

With all of my patches in this bug applied on top of alder, I'm able to successfully link libxul.
(Assignee)

Updated

6 years ago
Assignee: rjesup → ted.mielczarek
(Assignee)

Updated

6 years ago
Attachment #564904 - Attachment is obsolete: true
(Assignee)

Comment 42

6 years ago
Created attachment 586195 [details] [diff] [review]
generate Makefiles for WebRTC from GYP files at configure time

This patch lets us generate the Makefiles from the gyp files directly
into the objdir while running configure. This worked in a clobber
build for me on Linux, I'm running it through a Mac clobber right now
as well. If this works okay on Mac/Linux, then I think we should get
it landed on alder, and I'll continue to work on Windows support.
(Assignee)

Updated

6 years ago
Attachment #585507 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Depends on: 718031
(Assignee)

Updated

6 years ago
Attachment #564763 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #567607 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #584994 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #585508 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #585509 - Attachment is obsolete: true
(Assignee)

Comment 43

6 years ago
Landed a combined version of this on alder:
http://hg.mozilla.org/projects/alder/rev/75ef608c54d2

It's working on Linux, and it was working on Mac (I need to re-test there, I forgot to do so before landing). Windows is close, but we still have just a few small issues to work through. jesup is going to merge mozilla-central into alder to pick up the changes that created the separate "gkmedia" library. We'll change things so that the WebRTC code gets linked into gkmedia, which should solve one of the remaining issues (the libjingle copy of expat having symbol conflicts with Mozilla's built-in expat on Windows).
(Assignee)

Comment 44

6 years ago
I merged mozilla-central in today, and landed a few tweaks:
http://hg.mozilla.org/projects/alder/rev/3a981cf610f7 (link WebRTC code into gkmedias instead of libxul)
http://hg.mozilla.org/projects/alder/rev/84616cd44046 (work around some upstream code on Windows that doesn't actually seem to link)

...and my build succeeds on Windows. I'm not sure if anything works, since I can't get fabrice's patches in bug 692955 to build, but it seems promising.
(Reporter)

Comment 45

6 years ago
Created attachment 601494 [details] [diff] [review]
Patch for python 2.5

Win32 build machines run python 2.5
(Reporter)

Comment 46

6 years ago
Comment on attachment 601494 [details] [diff] [review]
Patch for python 2.5

Win32 build machines are *still* python 2.5.  Really simply patch.

This patch is for alder, for mozmake.py, which itself has not yet been reviewed, so an rs= may be appropriate.  

(I'm asking since I've told people we'd be doing reviews on alder checkins now, since we now are getting ready to land non-imported code in volume and also have new-to-mozilla external commiters joining the effort.)
Attachment #601494 - Flags: review?(ted.mielczarek)
Attachment #601494 - Flags: review?(khuey)
(Reporter)

Updated

6 years ago
Attachment #601494 - Flags: review?(khuey) → feedback?(khuey)
Attachment #601494 - Flags: feedback?(khuey) → feedback+
(Assignee)

Comment 47

5 years ago
Comment on attachment 601494 [details] [diff] [review]
Patch for python 2.5

Review of attachment 601494 [details] [diff] [review]:
-----------------------------------------------------------------

Gross, but thanks.
Attachment #601494 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 48

5 years ago
This is building on all our Tier-1 platforms on Tinderbox now (Android is busted, but we'll deal with that separately).
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Build Makefile.in's for major WebRTC modules → Build Makefiles from WebRTC GYP files
Were the {C,CPP}FLAGS issues solved?
(Assignee)

Comment 50

5 years ago
Yes, jesup patched that in bug 737398 by making the GYPfiles specify essential flags in something like a moz_cflags variable, and ignoring the default flags.
You need to log in before you can comment on or make changes to this bug.