Closed
Bug 688187
Opened 13 years ago
Closed 13 years ago
Build Makefiles from WebRTC GYP files
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jesup, Assigned: ted)
References
Details
Attachments
(2 files, 20 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ted
:
review+
khuey
:
feedback+
|
Details | Diff | Splinter Review |
Probably one Makefile for each section
Comment 1•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
What is libprotobuf actually being used for?
Comment 5•13 years ago
|
||
Looks like the audio_processing module is using it for debug tracing.
Reporter | ||
Comment 6•13 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 7•13 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
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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 10•13 years ago
|
||
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 11•13 years ago
|
||
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 12•13 years ago
|
||
Comment on attachment 563245 [details] [diff] [review]
work in progress v2
Didn't get this finished today, but updating with work in progress.
Comment 13•13 years ago
|
||
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 14•13 years ago
|
||
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 15•13 years ago
|
||
This builds some parts of the tree, but I still can't link code using it.
Attachment #563269 -
Attachment is obsolete: true
Comment 16•13 years ago
|
||
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 17•13 years ago
|
||
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 18•13 years ago
|
||
Reporter | ||
Comment 19•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Attachment #564372 -
Attachment is obsolete: true
Reporter | ||
Comment 20•13 years ago
|
||
Gets to actual link errors for libxul
Attachment #564470 -
Attachment is obsolete: true
Reporter | ||
Comment 21•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Attachment #563346 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #563598 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #563622 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #564483 -
Attachment is obsolete: true
Reporter | ||
Comment 22•13 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
Comment 23•13 years ago
|
||
ac_add_options --enable-webrtc is useless, since it's enabled by default.
You can disable it with --disable-webrtc
Comment 24•13 years ago
|
||
to build with the current alder repository, you also need to export OBJDIR.
Comment 25•13 years ago
|
||
Adding more header files to EXPORTS
Reporter | ||
Comment 26•13 years ago
|
||
Reporter | ||
Comment 27•13 years ago
|
||
Note: fabrice's patches for camera access (on another bug) fail to build for mac
Assignee | ||
Comment 28•13 years ago
|
||
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 29•13 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 30•13 years ago
|
||
Yeah, running it manually (or as part of a script) on import/gyp change was my intention.
Assignee | ||
Comment 31•13 years ago
|
||
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•13 years ago
|
Attachment #583771 -
Attachment is obsolete: true
Assignee | ||
Comment 32•13 years ago
|
||
This patch is also necessary to make the build succeed. It just tweaks an include path for the vp8 build.
Assignee | ||
Comment 33•13 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•13 years ago
|
Attachment #584794 -
Attachment is obsolete: true
Assignee | ||
Comment 34•13 years ago
|
||
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 35•13 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 36•13 years ago
|
||
Just a checkpoint.
Assignee | ||
Comment 37•13 years ago
|
||
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 38•13 years ago
|
||
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•13 years ago
|
Assignee: rjesup → ted.mielczarek
Assignee | ||
Updated•13 years ago
|
Attachment #564904 -
Attachment is obsolete: true
Assignee | ||
Comment 39•13 years ago
|
||
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•13 years ago
|
Attachment #585507 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #564763 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #567607 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #584994 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #585508 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #585509 -
Attachment is obsolete: true
Assignee | ||
Comment 40•13 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 41•13 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 42•13 years ago
|
||
Win32 build machines run python 2.5
Reporter | ||
Comment 43•13 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•13 years ago
|
Attachment #601494 -
Flags: review?(khuey) → feedback?(khuey)
Attachment #601494 -
Flags: feedback?(khuey) → feedback+
Assignee | ||
Comment 44•13 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 45•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Summary: Build Makefile.in's for major WebRTC modules → Build Makefiles from WebRTC GYP files
Comment 46•13 years ago
|
||
Were the {C,CPP}FLAGS issues solved?
Assignee | ||
Comment 47•13 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.
Description
•