688187 - Build Makefiles from WebRTC GYP files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Probably one Makefile for each section

Comment 1

[Timothy B. Terriberry (:derf)]

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.

Comment 2

[Ralph Giles (:rillian)]

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.

Comment 3

[Ralph Giles (:rillian)]

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

Comment 4

[Timothy B. Terriberry (:derf)]

What is libprotobuf actually being used for?

Comment 5

[Ralph Giles (:rillian)]

Looks like the audio_processing module is using it for debug tracing.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 11

[Ralph Giles (:rillian)]

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.

Comment 12

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 13

[Ralph Giles (:rillian)]

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.

Comment 14

[Ralph Giles (:rillian)]

Attachment: 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

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 563245 [details] [diff] [review]
work in progress v2

Didn't get this finished today, but updating with work in progress.

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: 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.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: 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.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: updated WIP

This builds some parts of the tree, but I still can't link code using it.

Comment 19

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 20

[[:fabrice] Fabrice Desré]

Attachment: 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.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add makefiles that invoke the native webrtc build system - WIP

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch to use gyp directly and put object files in OBJDIR - WIP

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch that gets to the link phase and fails - WIP

Gets to actual link errors for libxul

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: build with webrtc - works, all-in-one

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 26

[[:fabrice] Fabrice Desré]

ac_add_options --enable-webrtc is useless, since it's enabled by default.
You can disable it with --disable-webrtc

Comment 27

[[:fabrice] Fabrice Desré]

to build with the current alder repository, you also need to export OBJDIR.

Comment 28

[[:fabrice] Fabrice Desré]

Attachment: followup

Adding more header files to EXPORTS

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patches to make Mac build

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 31

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

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!

Comment 33

[(not currently active) Ted Mielczarek]

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

Comment 34

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 35

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 36

[(not currently active) Ted Mielczarek]

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.

Comment 37

[(not currently active) Ted Mielczarek]

Attachment: 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...

Comment 38

[(not currently active) Ted Mielczarek]

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

Comment 39

[(not currently active) Ted Mielczarek]

Attachment: generate Makefile.in build files for WebRTC from GYP files

Just a checkpoint.

Comment 40

[(not currently active) Ted Mielczarek]

Attachment: 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.)

Comment 41

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 42

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 43

[(not currently active) Ted Mielczarek]

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

Comment 44

[(not currently active) Ted Mielczarek]

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.

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch for python 2.5

Win32 build machines run python 2.5

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 47

[(not currently active) Ted Mielczarek]

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

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

Gross, but thanks.

Comment 48

[(not currently active) Ted Mielczarek]

This is building on all our Tier-1 platforms on Tinderbox now (Android is busted, but we'll deal with that separately).

Comment 49

[Mike Hommey [:glandium]]

Were the {C,CPP}FLAGS issues solved?

Comment 50

[(not currently active) Ted Mielczarek]

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.