Closed
Bug 741737
Opened 12 years ago
Closed 12 years ago
Configure libvpx with pkgconfig
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla14
People
(Reporter: stransky, Assigned: stransky)
References
Details
Attachments
(1 file, 3 obsolete files)
4.38 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #722127 +++ Patch for libvpx detection with pkgconfig
Attachment #611769 -
Flags: review?(tterribe)
Comment 1•12 years ago
|
||
I cannot find libvpx.pc in the upstream libvpx source. It's added as a separate source in Fedora SRPM. Upstream source generates vpc.pc which is defined by libs.mk (and not multilib friendly).
Assignee | ||
Comment 2•12 years ago
|
||
yeah, you're right, it should be vpx.pc instead of libvpx.pc.
Assignee | ||
Comment 3•12 years ago
|
||
Patch for vpx.pc (isntead of the previous for libvpx.pc).
Attachment #611769 -
Attachment is obsolete: true
Attachment #611769 -
Flags: review?(tterribe)
Attachment #611799 -
Flags: review?(tterribe)
Comment 4•12 years ago
|
||
Comment on attachment 611799 [details] [diff] [review] patch, v2 Review of attachment 611799 [details] [diff] [review]: ----------------------------------------------------------------- r- because of the version check issue, but if you fix that and ask for re-review, feel free to ask for it directly from a build peer like khuey@kylehuey.com or ted.mielczarek@gmail.com (they're the ones who'll ultimately need to approve of the changes, not me). ::: configure.in @@ +5669,5 @@ > + dnl ================================== > + dnl === libvpx Version check === > + dnl ================================== > + dnl Check to see if the system libvpx package is compiled with > + dnl VPX_CODEC_USE_INPUT_FRAGMENTS enabled. So, maybe I'm confused, but I thought the point of using pkg-config was that we wouldn't have to rely on terrible hacks like this #define check to enforce a minimum version. Or at least that's the motivation to take this patch for me. You should be able to do it right in the PKG_CHECK_MODULES() call. Could you please make that change as well?
Attachment #611799 -
Flags: review?(tterribe) → review-
Assignee | ||
Comment 5•12 years ago
|
||
Ahh, yes, you're right, the version is included in the upstream vpx.pc file. I didn't expect it there due to the original comments in configure.in. I'll provide the updated version.
Assignee | ||
Comment 6•12 years ago
|
||
Would you like keep the compilation test or not? Because it's not necessary for the version check now.
Comment 7•12 years ago
|
||
(In reply to Martin Stránský from comment #6) > Would you like keep the compilation test or not? Because it's not necessary > for the version check now. No, I think it's okay to drop that.
Assignee | ||
Comment 8•12 years ago
|
||
Updated the version check and removed the build check.
Attachment #611799 -
Attachment is obsolete: true
Attachment #615655 -
Flags: review?(khuey)
Comment on attachment 615655 [details] [diff] [review] patch, v3 Review of attachment 615655 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: configure.in @@ +5589,5 @@ > dnl system libvpx Support > dnl ======================================================== > +MOZ_ARG_WITH_BOOL(system-libvpx, > +[ --with-system-libvpx Use system libvpx (located with pkgconfig)], > +MOZ_NATIVE_LIBVPX=1) Indent MOZ_NATIVE_LIBVPX 4 spaces. @@ +5610,5 @@ > + dnl ============================ > + dnl Check to see if we have a system libvpx package. > + PKG_CHECK_MODULES(LIBVPX, vpx >= 1.0.0) > + AC_SUBST([LIBVPX_CFLAGS]) > + AC_SUBST([LIBVPX_LIBS]) Usually we don't bother with [] here unless they're needed, but I don't care much either way. @@ +5612,5 @@ > + PKG_CHECK_MODULES(LIBVPX, vpx >= 1.0.0) > + AC_SUBST([LIBVPX_CFLAGS]) > + AC_SUBST([LIBVPX_LIBS]) > + > + MOZ_CHECK_HEADERS([vpx/vpx_decoder.h], [], Use MOZ_CHECK_HEADER here. @@ +5616,5 @@ > + MOZ_CHECK_HEADERS([vpx/vpx_decoder.h], [], > + [AC_MSG_ERROR([Couldn't find vpx/vpx_decoder.h which is required for build with system libvpx. Use --without-system-libvpx to build with in-tree libvpx.])]) > + > + AC_CHECK_LIB(vpx, vpx_codec_dec_init_ver, [], > + [--with-system-libvpx requested but symbol vpx_codec_dec_init_ver not found]) Shouldn't this be AC_MSG_ERROR([--with-system-libvpx ...]) for the fourth arg?
Attachment #615655 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Thanks, this one address the comments.
Attachment #615704 -
Flags: checkin?
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #615655 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Comment on attachment 615704 [details] [diff] [review] patch for check-in You don't need to worry about the checkin? flag. checkin-needed is all you need.
Attachment #615704 -
Flags: checkin?
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eee73897136b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•