Closed Bug 757637 Opened 13 years ago Closed 12 years ago

Rollup of make system changes for WebRTC

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 4 obsolete files)

Rollup media/webrtc/trunk changes from webrtc.org drop
83.78 KB, patch
derf
: review+
ted
: review+
Details | Diff | Splinter Review
Updated rollup makesystem patch (v2)
41.98 KB, patch
khuey
: review-
ted
: review+
glandium
: feedback+
Details | Diff | Splinter Review
updates against previous patch per review
25.95 KB, patch
khuey
: review+
Details | Diff | Splinter Review
This rolls up the majority of the makesystem changes for WebRTC, in particular the gyp->Makefile converter ted wrote in bug 643692. Note that the current patch uploaded here has the default as webrtc enabled, and before landing I plan to switch it disabled. Also the patch includes generation of the makefiles for media/webrtc/signaling, which will land in a later traunch, and so will be removed before landing. This is not a standalone patch, it's being used to partition out the code for individual reviews. There will be further patches such as the changes to the webrtc gyp files to work with this. This covers everything outside of media (and includes media/webrtc/Makefile.in and shared_libs.mk)
Comment on attachment 626213 [details] [diff] [review] Rollup patch for gyp->Makefile generator, etc Looking for initial review; I expect to have issues to deal with. This is the first primary blocker to initial preffed-off landing of webrtc. (ducks for cover)
Attachment #626213 - Flags: review?(khuey)
Attachment #626213 - Flags: feedback?(mh+mozilla)
Does not include windows video capture code or any third_party/* modules Mostly makesystem code, a few bugfixes, and also libvpx interface-level changes
The second patch has the coordinating changes to gyp for mozmake (os.sep, etc), along with changes to the gyp files themselves (filtering out cflags except for one or two exceptions, etc).
Comment on attachment 626213 [details] [diff] [review] Rollup patch for gyp->Makefile generator, etc Review of attachment 626213 [details] [diff] [review]: ----------------------------------------------------------------- ::: client.mk @@ +253,5 @@ > $(wildcard $(TOPSRCDIR)/build/autoconf/*.m4) \ > $(TOPSRCDIR)/js/src/aclocal.m4 \ > + $(wildcard $(TOPSRCDIR)/media/webrtc/build/autoconf/*.m4) \ > + $(TOPSRCDIR)/media/webrtc/signaling/signaling.gyp \ > + $(TOPSRCDIR)/media/webrtc/trunk/test/metrics.gyp \ EXTRA_CONFIG_DEPS is used as a dependency to rebuild configure and js/src/configure. Why do you want to refresh them when a gyp file changes? It's running configure that would be necessary, but then, it would be better to have an equivalent to make-makefile and remake them individually, like the rules re-making Makefiles. ::: config/rules.mk @@ +643,5 @@ > endif # LIBRARY_NAME > > +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS))) > +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only) > +#endif Commenting this out is not a very good idea right now. However, this is likely going to go away with bug 757339. ::: toolkit/library/Makefile.in @@ +590,5 @@ > endif > > +ifdef MOZ_WEBRTC > +ifeq ($(OS_TARGET),Linux) > +OS_LIBS += -lexpat Do we really want to use system expat when we have ours?
Attachment #626213 - Flags: feedback?(mh+mozilla) → feedback-
Assignee: nobody → rjesup
(In reply to Mike Hommey [:glandium] from comment #4) > Comment on attachment 626213 [details] [diff] [review] > Rollup patch for gyp->Makefile generator, etc > > Review of attachment 626213 [details] [diff] [review]: Thanks for the quick response > ----------------------------------------------------------------- > > ::: client.mk > @@ +253,5 @@ > > $(wildcard $(TOPSRCDIR)/build/autoconf/*.m4) \ > > $(TOPSRCDIR)/js/src/aclocal.m4 \ > > + $(wildcard $(TOPSRCDIR)/media/webrtc/build/autoconf/*.m4) \ > > + $(TOPSRCDIR)/media/webrtc/signaling/signaling.gyp \ > > + $(TOPSRCDIR)/media/webrtc/trunk/test/metrics.gyp \ > > EXTRA_CONFIG_DEPS is used as a dependency to rebuild configure and > js/src/configure. Why do you want to refresh them when a gyp file changes? > It's running configure that would be necessary, but then, it would be better > to have an equivalent to make-makefile and remake them individually, like > the rules re-making Makefiles. It might, but note that we'd have to track the in-gypfile dependencies as well (they include each other). While annoying (somewhat) having it rebuild all the makefiles when any of the gyp files changes is safe and guaranteed not to miss any dependencies, instead of spending a lot of time building a dependency-parser for gyp files. And gyp->makefile is fast, though on Windows configure itself is slow for no good reason. I'd be good with filing a bug to eventually replace this with something fancier. > > ::: config/rules.mk > @@ +643,5 @@ > > endif # LIBRARY_NAME > > > > +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS))) > > +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only) > > +#endif > > Commenting this out is not a very good idea right now. However, this is > likely going to go away with bug 757339. > > ::: toolkit/library/Makefile.in > @@ +590,5 @@ > > endif > > > > +ifdef MOZ_WEBRTC > > +ifeq ($(OS_TARGET),Linux) > > +OS_LIBS += -lexpat > > Do we really want to use system expat when we have ours? Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle or at least the major portion of it, which we plan to do). Right now sippcc (signalling library to replace jingle) uses it too, to be seen if we can eliminate that use. Initial landing will not include libjingle or sipcc.
(In reply to Randell Jesup [:jesup] from comment #5) > > Do we really want to use system expat when we have ours? > > Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle > or at least the major portion of it, which we plan to do). Right now sippcc > (signalling library to replace jingle) uses it too, to be seen if we can > eliminate that use. Initial landing will not include libjingle or sipcc. Why do you need it on linux only, then?
(In reply to Mike Hommey [:glandium] from comment #6) > (In reply to Randell Jesup [:jesup] from comment #5) > > > Do we really want to use system expat when we have ours? > > > > Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle > > or at least the major portion of it, which we plan to do). Right now sippcc > > (signalling library to replace jingle) uses it too, to be seen if we can > > eliminate that use. Initial landing will not include libjingle or sipcc. > > Why do you need it on linux only, then? Windows doesn't have it (we build a copy in third_party), and Mac should use it but for some odd reason it doesn't work (need to dig up the bug on that; caused a lot of pain). Eventually we should use it on Mac as well.
I didn't add that bit to client.mk, that should be unnecessary. The gyp backend writes out proper dependencies to regenerate Makefiles as-needed. The expat situation is unfortunate, but as jesup described it's due to our expat being UTF-16 and libjingle wanting a UTF-8 one. I believe Windows does build the in-WebRTC-tree copy, and I don't remember what happens on Mac.
Comment on attachment 626213 [details] [diff] [review] Rollup patch for gyp->Makefile generator, etc Review of attachment 626213 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py @@ +99,5 @@ > +endif > + > +# Rules for regenerating Makefiles from GYP files. > +Makefile: %(input_gypfiles)s %(generator)s > + $(PYTHON) %(commandline)s Here's the bit with the rules for regenerating Makefiles from GYP files. @@ +432,5 @@ > + "--toplevel-dir=$(topsrcdir)", > + #XXX: handle other generator_flags gracefully? > + "-G OBJDIR=$(DEPTH)"] + \ > + ['-D%s' % d for d in options.defines] + \ > + [topsrcdir_path(b) for b in params['build_files']] You can see here where we cobble the commandline back together to stick it in the Makefile.
(In reply to Mike Hommey [:glandium] from comment #4) > > +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS))) > > +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only) > > +#endif > > Commenting this out is not a very good idea right now. However, this is > likely going to go away with bug 757339. I have to retract on this. SHARED_LIBRARY_LIBS is used for static libraries, too, and putting things in there that are not libraries is wrong. Use EXTRA_DSO_LDOPTS instead.
Aha. The client.mk thing was a temporary hack until the gypfile dependency was in place; I'd forgotten that and when I saw it I assumed that was how ted had handled it (seemed rather bear-skins and stone-knives, and that's because it is). I'll simply remove that.
Attached patch Updated rollup makesystem patch (obsolete) — — Splinter Review
Incorporates patch 626550 to remove old hacks
Attachment #626213 - Attachment is obsolete: true
Attachment #626213 - Flags: review?(khuey)
Attachment #626560 - Flags: review?(khuey)
Attachment #626560 - Flags: feedback?(mh+mozilla)
Comment on attachment 626228 [details] [diff] [review] Rollup media/webrtc/trunk changes from webrtc.org drop r? to derf for the vp8 parts, khuey for the rest. The ALSA stuff is only until we import the fix from upstream (they finally fixed it; issue 450 I think at webrtc.org).
Attachment #626228 - Flags: review?(tterribe)
Attachment #626228 - Flags: review?(khuey)
Comment on attachment 626228 [details] [diff] [review] Rollup media/webrtc/trunk changes from webrtc.org drop Review of attachment 626228 [details] [diff] [review]: ----------------------------------------------------------------- I've upgraded m-c to libvpx 1.0.0 now, so we may no longer need all the libvpx build changes, but there isn't actually anything wrong with them, so I'm fine giving an r+. ::: media/webrtc/trunk/src/modules/video_coding/codecs/vp8/main/source/vp8.gypi @@ +39,5 @@ > + '$(DIST)/include', > + ], > + 'defines': [ > + # This must be updated to match mozilla's version of libvpx > + 'WEBRTC_LIBVPX_VERSION=971', You should be able to set this to 1000 (v1.0.0) now. @@ +40,5 @@ > + ], > + 'defines': [ > + # This must be updated to match mozilla's version of libvpx > + 'WEBRTC_LIBVPX_VERSION=971', > + 'WEBRTC_LIBVPX_TEMPORAL_LAYERS=0' You should also be able to set this to 1 now. Perhaps that means you don't actually need this as a separate #define now.
Attachment #626228 - Flags: review?(tterribe) → review+
Comment on attachment 626560 [details] [diff] [review] Updated rollup makesystem patch Asking r? to ted for parts not written by ted (or convert to f+/- if you prefer)
Attachment #626560 - Flags: review?(ted.mielczarek)
Comment on attachment 626228 [details] [diff] [review] Rollup media/webrtc/trunk changes from webrtc.org drop Asking r? to ted for parts not written by ted (or convert to f+/- if you prefer). Or ted and khuey can have a cage match to see who gets to escape reviewing it. :-) This is largely changes to gyp that I did (and the alsa fix waiting for us to pick up the upstream fix).
Attachment #626228 - Flags: review?(ted.mielczarek)
Very minor update to previous patch to remove JPEG_LIBS from gkmedia link now that jpeg is in gkmedia always. (See bug 758413) And I noticed that the old license is on the Makefile.in, and there's no license on mozmake.py (but I doubt if it should be, as the file is derived from make.py under gyp's license, even though it's a new file).
Attachment #626550 - Attachment is obsolete: true
Attachment #626560 - Attachment is obsolete: true
Attachment #626560 - Flags: review?(ted.mielczarek)
Attachment #626560 - Flags: review?(khuey)
Attachment #626560 - Flags: feedback?(mh+mozilla)
Attachment #627128 - Flags: review?(ted.mielczarek)
Attachment #627128 - Flags: review?(khuey)
Attachment #627128 - Flags: feedback?(mh+mozilla)
Attachment #627128 - Flags: feedback?(mh+mozilla) → feedback+
(In reply to Randell Jesup [:jesup] from comment #18) > And I noticed that the old license is on the Makefile.in, and there's no > license on mozmake.py (but I doubt if it should be, as the file is derived > from make.py under gyp's license, even though it's a new file). There should be a license header in mozmake.py, *especially* if it's not MPL.
We should copy the BSD license headers from the other gyp code. The rest of GYP is BSD-licensed, and this is functionally a part of GYP, so it should be too.
Comment on attachment 626228 [details] [diff] [review] Rollup media/webrtc/trunk changes from webrtc.org drop Review of attachment 626228 [details] [diff] [review]: ----------------------------------------------------------------- I don't think there's anything here that I need to review.
Attachment #626228 - Flags: review?(khuey)
Comment on attachment 627128 [details] [diff] [review] Updated rollup makesystem patch (v2) Review of attachment 627128 [details] [diff] [review]: ----------------------------------------------------------------- r- because I'd like to see this again. Also, we need to figure out what to do about expat. I also haven't looked at the gyp bits in detail, but I'm inclined not to look too hard. ::: configure.in @@ +716,5 @@ > AC_DEFINE(_CRT_SECURE_NO_WARNINGS) > AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS) > elif test "$_CC_MAJOR_VERSION" = "17"; then > _CC_SUITE=11 > + _MSVS_VERSION=2011 Yuck yuck yuck. Why does gyp care what version of VS this is? @@ +5522,5 @@ > + MOZ_WEBM=1 > + MOZ_RAW=1 > + MOZ_VP8_ENCODER=1 > + MOZ_VP8_ERROR_CONCEALMENT=1 > +fi We need code that makes sure that someone who passes --disable-media also passes --disable-webrtc. Ditto for the other things in here. Also, why does WebRTC need MOZ_RAW? @@ +8978,5 @@ > + if test "${target_cpu}" = "x86_64"; then > + OS_BITS=64 > + else > + OS_BITS=32 > + fi Just test HAVE_64BIT_OS. @@ +8991,5 @@ > + -D include_internal_video_render=0 \ > + $EXTRA_GYP_DEFINES \ > + --generator-output=${_objdir}/media/webrtc/trunk \ > + --toplevel-dir=${srcdir} \ > + -G OBJDIR=${_objdir} \ We repeat these defines 3 times. Lets put them in a single variable and use that. ::: layout/media/symbols.def.in @@ +154,5 @@ > +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z > +?Create@VideoEngine@webrtc@@SAPAV12@XZ > +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z > +#endif > +#endif Unless we absolutely have to do this, we should avoid it. Can we add declspec dllexport/import to the relevant stuff? ::: media/webrtc/Makefile.in @@ +33,5 @@ > +# and other provisions required by the GPL or the LGPL. If you do not delete > +# the provisions above, a recipient may use your version of this file under > +# the terms of any one of the MPL, the GPL or the LGPL. > +# > +# ***** END LICENSE BLOCK ***** MPL 2! ::: media/webrtc/shared_libs.mk @@ +1,1 @@ > +# shared libs for webrtc This needs a license header. ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py @@ +1,1 @@ > +# Python 2.5 needs this for the with statement. This needs a license header. ::: toolkit/library/Makefile.in @@ +452,5 @@ > ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa) > CXXFLAGS += $(TK_CFLAGS) > OS_LIBS += \ > -framework SystemConfiguration \ > + -framework QTKit \ I don't know enough about Mac to know if this is kosher. @@ +592,5 @@ > +ifdef MOZ_WEBRTC > +ifeq ($(OS_TARGET),Linux) > +OS_LIBS += -lexpat > +endif > +endif This makes me very sad. We should avoid this if at all possible.
Attachment #627128 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22) > Comment on attachment 627128 [details] [diff] [review] > Updated rollup makesystem patch (v2) > > Review of attachment 627128 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- because I'd like to see this again. Also, we need to figure out what to > do about expat. Thanks! I think I have answers for most of this. > I also haven't looked at the gyp bits in detail, but I'm inclined not to > look too hard. Sure. And glandium has looked at some of those (compiler flags, etc). We basically ignore webrtc's compiler flags except for a few rare cases like sse2, and ted can look too. > > ::: configure.in > @@ +716,5 @@ > > AC_DEFINE(_CRT_SECURE_NO_WARNINGS) > > AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS) > > elif test "$_CC_MAJOR_VERSION" = "17"; then > > _CC_SUITE=11 > > + _MSVS_VERSION=2011 > > Yuck yuck yuck. Why does gyp care what version of VS this is? trunk/build/common.gypi turns on and off compiler options (multi-core, incremental linking on 32-bit, pre-compiled headers) > > @@ +5522,5 @@ > > + MOZ_WEBM=1 > > + MOZ_RAW=1 > > + MOZ_VP8_ENCODER=1 > > + MOZ_VP8_ERROR_CONCEALMENT=1 > > +fi > > We need code that makes sure that someone who passes --disable-media also > passes --disable-webrtc. Ditto for the other things in here. Gotcha. Most of these are mandatory for webrtc, so we need to handle configure conflicts - what's the standard; kick out and say "don't do that"? > Also, why does WebRTC need MOZ_RAW? I believe the reason is so that video elements can work on raw YUV data. It's possible this isn't really needed. Derf? > @@ +8978,5 @@ > > + if test "${target_cpu}" = "x86_64"; then > > + OS_BITS=64 > > + else > > + OS_BITS=32 > > + fi > > Just test HAVE_64BIT_OS. Ok. > @@ +8991,5 @@ > > + -D include_internal_video_render=0 \ > > + $EXTRA_GYP_DEFINES \ > > + --generator-output=${_objdir}/media/webrtc/trunk \ > > + --toplevel-dir=${srcdir} \ > > + -G OBJDIR=${_objdir} \ > > We repeat these defines 3 times. Lets put them in a single variable and use > that. Ok. > ::: layout/media/symbols.def.in > @@ +154,5 @@ > > +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z > > +?Create@VideoEngine@webrtc@@SAPAV12@XZ > > +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z > > +#endif > > +#endif > > Unless we absolutely have to do this, we should avoid it. Can we add > declspec dllexport/import to the relevant stuff? I can try, but last time I looked it didn't seem to do what we needed (and I think that's why symbols.def.in exists...) Most are .cc files; cairo puts everything in extern "C" - and they still all need to be listed there. extern "C" would make it more readable, and avoid having to have two versions of each. This will require mods to the main code; that's probably not a problem. > ::: media/webrtc/Makefile.in > @@ +33,5 @@ > > +# and other provisions required by the GPL or the LGPL. If you do not delete > > +# the provisions above, a recipient may use your version of this file under > > +# the terms of any one of the MPL, the GPL or the LGPL. > > +# > > +# ***** END LICENSE BLOCK ***** > > MPL 2! Yup! (Knew we needed to revise.) > ::: media/webrtc/shared_libs.mk > @@ +1,1 @@ > > +# shared libs for webrtc > > This needs a license header. Ok. > ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py > @@ +1,1 @@ > > +# Python 2.5 needs this for the with statement. > > This needs a license header. This one is derived from /make.py so I assume it falls into the BSD-ish license that's under, so I don't know we add an MPL header. > ::: toolkit/library/Makefile.in > @@ +452,5 @@ > > ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa) > > CXXFLAGS += $(TK_CFLAGS) > > OS_LIBS += \ > > -framework SystemConfiguration \ > > + -framework QTKit \ > > I don't know enough about Mac to know if this is kosher. Required to get to the A/V capture code on Mac. > @@ +592,5 @@ > > +ifdef MOZ_WEBRTC > > +ifeq ($(OS_TARGET),Linux) > > +OS_LIBS += -lexpat > > +endif > > +endif > > This makes me very sad. We should avoid this if at all possible. Already removed it (see comment I made in another bug on expat); the cost is a second copy of expat in our executable (and on built on Linux (Fedora 15), we still also dynamic link to expat as well, but the binaries probably won't). (We need a char* expat; xul's (which we can't access anyways on Windows) is PR_UniChar*)
(In reply to Randell Jesup [:jesup] from comment #23) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22) > > Comment on attachment 627128 [details] [diff] [review] > > Updated rollup makesystem patch (v2) > > > > Review of attachment 627128 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r- because I'd like to see this again. Also, we need to figure out what to > > do about expat. > > Thanks! I think I have answers for most of this. > > > I also haven't looked at the gyp bits in detail, but I'm inclined not to > > look too hard. > > Sure. And glandium has looked at some of those (compiler flags, etc). We > basically ignore webrtc's compiler flags except for a few rare cases like > sse2, and ted can look too. Good. > > > > ::: configure.in > > @@ +716,5 @@ > > > AC_DEFINE(_CRT_SECURE_NO_WARNINGS) > > > AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS) > > > elif test "$_CC_MAJOR_VERSION" = "17"; then > > > _CC_SUITE=11 > > > + _MSVS_VERSION=2011 > > > > Yuck yuck yuck. Why does gyp care what version of VS this is? > > trunk/build/common.gypi turns on and off compiler options (multi-core, > incremental linking on 32-bit, pre-compiled headers) Hmm, ok. I need to look at this harder next time. > > > > @@ +5522,5 @@ > > > + MOZ_WEBM=1 > > > + MOZ_RAW=1 > > > + MOZ_VP8_ENCODER=1 > > > + MOZ_VP8_ERROR_CONCEALMENT=1 > > > +fi > > > > We need code that makes sure that someone who passes --disable-media also > > passes --disable-webrtc. Ditto for the other things in here. > > Gotcha. Most of these are mandatory for webrtc, so we need to handle > configure conflicts - what's the standard; kick out and say "don't do that"? > > Also, why does WebRTC need MOZ_RAW? > > I believe the reason is so that video elements can work on raw YUV data. > It's possible this isn't really needed. Derf? > > > @@ +8978,5 @@ > > > + if test "${target_cpu}" = "x86_64"; then > > > + OS_BITS=64 > > > + else > > > + OS_BITS=32 > > > + fi > > > > Just test HAVE_64BIT_OS. > > Ok. > > > @@ +8991,5 @@ > > > + -D include_internal_video_render=0 \ > > > + $EXTRA_GYP_DEFINES \ > > > + --generator-output=${_objdir}/media/webrtc/trunk \ > > > + --toplevel-dir=${srcdir} \ > > > + -G OBJDIR=${_objdir} \ > > > > We repeat these defines 3 times. Lets put them in a single variable and use > > that. > > Ok. > > > ::: layout/media/symbols.def.in > > @@ +154,5 @@ > > > +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z > > > +?Create@VideoEngine@webrtc@@SAPAV12@XZ > > > +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z > > > +#endif > > > +#endif > > > > Unless we absolutely have to do this, we should avoid it. Can we add > > declspec dllexport/import to the relevant stuff? > > I can try, but last time I looked it didn't seem to do what we needed (and I > think that's why symbols.def.in exists...) Most are .cc files; cairo puts > everything in extern "C" - and they still all need to be listed there. > extern "C" would make it more readable, and avoid having to have two > versions of each. This will require mods to the main code; that's probably > not a problem. > > > ::: media/webrtc/Makefile.in > > @@ +33,5 @@ > > > +# and other provisions required by the GPL or the LGPL. If you do not delete > > > +# the provisions above, a recipient may use your version of this file under > > > +# the terms of any one of the MPL, the GPL or the LGPL. > > > +# > > > +# ***** END LICENSE BLOCK ***** > > > > MPL 2! > > Yup! (Knew we needed to revise.) > > > ::: media/webrtc/shared_libs.mk > > @@ +1,1 @@ > > > +# shared libs for webrtc > > > > This needs a license header. > > Ok. > > > ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py > > @@ +1,1 @@ > > > +# Python 2.5 needs this for the with statement. > > > > This needs a license header. > > This one is derived from /make.py so I assume it falls into the BSD-ish > license that's under, so I don't know we add an MPL header. > > > ::: toolkit/library/Makefile.in > > @@ +452,5 @@ > > > ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa) > > > CXXFLAGS += $(TK_CFLAGS) > > > OS_LIBS += \ > > > -framework SystemConfiguration \ > > > + -framework QTKit \ > > > > I don't know enough about Mac to know if this is kosher. > > Required to get to the A/V capture code on Mac. > > > @@ +592,5 @@ > > > +ifdef MOZ_WEBRTC > > > +ifeq ($(OS_TARGET),Linux) > > > +OS_LIBS += -lexpat > > > +endif > > > +endif > > > > This makes me very sad. We should avoid this if at all possible. > > Already removed it (see comment I made in another bug on expat); the cost is > a second copy of expat in our executable (and on built on Linux (Fedora 15), > we still also dynamic link to expat as well, but the binaries probably > won't). (We need a char* expat; xul's (which we can't access anyways on > Windows) is PR_UniChar*) Uh, that's a pain.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #24) > > > > > > ::: configure.in > > > @@ +716,5 @@ > > > > AC_DEFINE(_CRT_SECURE_NO_WARNINGS) > > > > AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS) > > > > elif test "$_CC_MAJOR_VERSION" = "17"; then > > > > _CC_SUITE=11 > > > > + _MSVS_VERSION=2011 > > > > > > Yuck yuck yuck. Why does gyp care what version of VS this is? > > > > trunk/build/common.gypi turns on and off compiler options (multi-core, > > incremental linking on 32-bit, pre-compiled headers) > > Hmm, ok. I need to look at this harder next time. You may not need to: I believe these only apply to standalone executables generated via gyp I think; mostly test code. This is the standard chromium common.gypi I think, or at least some snapshot of it. I think we have another older snapshot (unused) in our tree in the ipc dir tree. > > > @@ +592,5 @@ > > > > +ifdef MOZ_WEBRTC > > > > +ifeq ($(OS_TARGET),Linux) > > > > +OS_LIBS += -lexpat > > > > +endif > > > > +endif > > > > > > This makes me very sad. We should avoid this if at all possible. > > > > Already removed it (see comment I made in another bug on expat); the cost is > > a second copy of expat in our executable (and on built on Linux (Fedora 15), > > we still also dynamic link to expat as well, but the binaries probably > > won't). (We need a char* expat; xul's (which we can't access anyways on > > Windows) is PR_UniChar*) > > Uh, that's a pain. Yeah. We need expat right now for signaling (libjingle and the replacement for most of it, sipcc (media/webrtc/signaling) both use it). I'll circle with the sipcc people to see if we can (as we move toward full WebRTC) remove the sipcc parts that need expat. Otherwise because of the gkmedia thing we're stuck with either two copies, or -lexpat on linux/mac and two copies on Windows.
> > + MOZ_WEBM=1 > > + MOZ_RAW=1 > > + MOZ_VP8_ENCODER=1 > > + MOZ_VP8_ERROR_CONCEALMENT=1 > > +fi > > We need code that makes sure that someone who passes --disable-media also > passes --disable-webrtc. Ditto for the other things in here. MOZ_MEDIA, MOZ_VP8_ENCODER and MOZ_VP8_ERROR_CONCEALMENT have no --enable or disable options; they're turned on as needed by other options. --disable-raw will already turn off MOZ_RAW (it's after MOZ_WEBRTC) --disable-webm will (soon) turn off MOZ_WEBM without turning off VP8
Comment on attachment 631070 [details] [diff] [review] updates against previous patch per review Switched default to preffed off (per landing plan) mozmake.py doesn't seem to actually be derived from make.py, so I licensed that. I broke out webm from webrtc, which required being able to build libvpx without webm - derf, please look at that. I've done builds with all the various combinations. And while not shown here, I also removed -lexpat from linux per my comment
Attachment #631070 - Flags: review?(tterribe)
Attachment #631070 - Flags: review?(khuey)
Comment on attachment 631070 [details] [diff] [review] updates against previous patch per review Review of attachment 631070 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/src/build/common.gypi @@ +137,5 @@ > }], > ['OS=="win"', { > 'defines': [ > 'WEBRTC_WIN', > + 'WEBRTC_EXPORT', This is experimental to try to provoke _declspec(dllexport) on some of the symbols we need to export
OS: Windows 2000 → All
Version: Other Branch → Trunk
Comment on attachment 631070 [details] [diff] [review] updates against previous patch per review Review of attachment 631070 [details] [diff] [review]: ----------------------------------------------------------------- LTGM. ::: config/autoconf.mk.in @@ +145,5 @@ > MOZ_OMX_PLUGIN = @MOZ_OMX_PLUGIN@ > MOZ_GSTREAMER = @MOZ_GSTREAMER@ > MOZ_VP8_ERROR_CONCEALMENT = @MOZ_VP8_ERROR_CONCEALMENT@ > MOZ_VP8_ENCODER = @MOZ_VP8_ENCODER@ > +MOZ_VP8 = @MOZ_VP8@ I'd prefer to have this above the other two VP8 defines here (it's a logical progression: decoder, better decoder, encoder), but that's no big deal. ::: configure.in @@ +4551,4 @@ > MOZ_MEDIA_PLUGINS= > MOZ_MEDIA_NAVIGATOR= > MOZ_OMX_PLUGIN= > +MOZ_VP8= Like here. @@ +8842,5 @@ > AC_SUBST(MOZ_MEDIA_PLUGINS) > AC_SUBST(MOZ_OMX_PLUGIN) > AC_SUBST(MOZ_VP8_ERROR_CONCEALMENT) > AC_SUBST(MOZ_VP8_ENCODER) > +AC_SUBST(MOZ_VP8) Same here.
Attachment #631070 - Flags: review?(tterribe) → review+
I'd still like to leave mozmake.py as BSD licensed since it runs as part of GYP. Even if we never upstream it, it's still weird for it to have a license different to the rest of GYP.
(In reply to Ted Mielczarek [:ted] from comment #31) > I'd still like to leave mozmake.py as BSD licensed since it runs as part of > GYP. Even if we never upstream it, it's still weird for it to have a license > different to the rest of GYP. Sure. Will do; that was my original assumption, I'll go back to that. Should I put a note to that effect in there? CCing gerv for his comments.
You can slap whatever license header GYP uses on there. I consider it a patch to upstream software, even if we don't actually upstream it.
Comment on attachment 626228 [details] [diff] [review] Rollup media/webrtc/trunk changes from webrtc.org drop Review of attachment 626228 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/peerconnection.gyp @@ +13,5 @@ > }, > > + # for mozilla, we want to force stuff to build but we don't want peerconnection_client or server > + # for unknown reasons, 'target' must be outside of conditions. And don't try to build a dummy > + # executable... I can't recall exactly why, but I think they wanted to pull in the protobuf stuff, and we had a hard time building that. @@ +26,5 @@ > + 'action_name': 'dummy', > + 'action': [ > + 'echo ARGHHHHHHHHHHHHHHHHHHHH', > + ], > + 'message': 'Generating scream', I LOLed at this a little. ::: media/webrtc/trunk/src/build/common.gypi @@ +66,5 @@ > 'webrtc_root%': '<(DEPTH)/third_party/webrtc', > }, { > # Settings for the standalone (not-in-Chromium) build. > > + 'include_pulse_audio%': 0, What's the plan for merging this with upstream? You have a lot of fiddly changes here. (Side note: the WebRTC standalone build seems like bullshit, since lots of things were broken.) ::: media/webrtc/trunk/src/build/getsdksamplesdir.py @@ +13,5 @@ > + print >>sys.stderr, "Could not locate Windows SDK Samples directory via the registry" > + return 1 > + > +if __name__ == '__main__': > + sys.exit(main()) I actually don't think we need this anymore since we rewrote all that code instead of using the stuff from the SDK. ::: media/webrtc/trunk/src/supplement.gypi @@ +1,2 @@ > #!/usr/bin/env python > # This file is generated by trunk/tools/create_supplement_gypi.py. Not for check-in. Does this get generated as part of the build? If so, we should not include it like it says. ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/make.py @@ +213,5 @@ > > # The source directory tree. > srcdir := %(srcdir)s > abs_srcdir := $(abspath $(srcdir)) > +os_sep := %(os_sep)s We should drop all these changes to make.py since we're not using it. ::: media/webrtc/trunk/tools/gyp/pylib/gyp/mac_tool.py @@ +40,5 @@ > > def ExecFlock(self, lockfile, *cmd_list): > """Emulates the most basic behavior of Linux's flock(1).""" > # Rely on exception handling to report errors. > + fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0666) Also this.
Attachment #626228 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 627128 [details] [diff] [review] Updated rollup makesystem patch (v2) Review of attachment 627128 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you address khuey's concerns. ::: media/webrtc/Makefile.in @@ +44,5 @@ > +include $(DEPTH)/config/autoconf.mk > + > +DIRS = trunk \ > + trunk/testing \ > + $(NULL) This should be: DIRS = \ trunk \ trunk/testing \ $(NULL) (two spaces, not tabs) ::: media/webrtc/shared_libs.mk @@ +1,3 @@ > +# shared libs for webrtc > +SHARED_LIBRARY_LIBS += \ > + $(call EXPAND_LIBNAME_PATH,jingle,$(DEPTH)/media/webrtc/trunk/third_party/libjingle/libjingle_libjingle) \ Two spaces, not tabs. ::: toolkit/toolkit-tiers.mk @@ +127,5 @@ > endif > > +ifdef MOZ_WEBRTC > +tier_platform_dirs += \ > + media/webrtc \ I know you're just copying what's already here, but two-space indent please.
Attachment #627128 - Flags: review?(ted.mielczarek) → review+
>::: media/webrtc/trunk/src/supplement.gypi >@@ +1,2 @@ >> #!/usr/bin/env python >> # This file is generated by trunk/tools/create_supplement_gypi.py. Not for check-in. > Does this get generated as part of the build? If so, we should not include it like it says. It gets generated when we check out the code from svn, so we really need to import it (and we need a modification anyways). The update procedure should pick up any upstream changes correctly and merge our change into the result. > - fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0o666) > + fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0666) I want to keep this (I'll upstream it as well). 0o666 is simply wrong.
That looks like valid Python to me: >>> 0o666 == 0666 True
(In reply to Ted Mielczarek [:ted] from comment #37) > That looks like valid Python to me: > >>> 0o666 == 0666 > True All I know is that I was getting errors building for Mac before I made that change (some time ago). Also: is it something that might be python version dependent? (I'm no python expert!) I can remove it and see what happens...
Ah, you're right, that's Python 2.6+.
No problem with mozmake.py being under the same licence as the rest of gyp; but please put a header on it. If a copyright line is part of the header, the copyright holder is the Mozilla Foundation. Gerv
Gerv: with all other Google projects, we assign copyright to Google. (We have a corporate contributor licensing agreement on file with them.) I don't know if it's that important in this case, since I have no immediate plans to upstream this code.
(In reply to Ted Mielczarek [:ted] from comment #41) > Gerv: with all other Google projects, we assign copyright to Google. (We > have a corporate contributor licensing agreement on file with them.) I don't > know if it's that important in this case, since I have no immediate plans to > upstream this code. I don't think we are actually assigning copyright to Google. We grant them a special license, but the copyright is still ours.
You may be right, but IANAL: http://code.google.com/legal/corporate-cla-v1.0.html "Subject to the terms and conditions of this Agreement, You hereby grant to Google and to recipients of software distributed by Google a perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable copyright license to reproduce, prepare derivative works of, publicly display, publicly perform, sublicense, and distribute Your Contributions and such derivative works."
A grant of rights is not the same as a copyright assignment, although often the difference is pretty much only this one: whose name appears on the copyright statement! Please use MoFo as copyright holder if there's a spot in the boilerplate for it. Gerv
Attachment #631070 - Attachment is obsolete: true
Attachment #631070 - Flags: review?(khuey)
Comment on attachment 633620 [details] [diff] [review] updates against previous patch per review Replace patch with one with a minor update to the mozmake.py copyright header per Gerv Already has r+ from derf on the VP8 bits, moving r? for kyle forward.
Attachment #633620 - Flags: review?(khuey)
(In reply to Randell Jesup [:jesup] from comment #46) > Comment on attachment 633620 [details] [diff] [review] > updates against previous patch per review > > Replace patch with one with a minor update to the mozmake.py copyright > header per Gerv Re-read gerv's comment, he said MoFo not MoCo; I'll revise before checkin (if it matters).
Yes, please say MoFo rather than MoCo. Gerv
https://hg.mozilla.org/mozilla-central/rev/a7376a73b1af https://hg.mozilla.org/mozilla-central/rev/a9aeaa818e64
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 771973
Blocks: 771981
No longer depends on: 771973
Whiteboard: [qa-]
Blocks: 1269165
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: