Closed Bug 741737 Opened 12 years ago Closed 12 years ago

Configure libvpx with pkgconfig

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #722127 +++

Patch for libvpx detection with pkgconfig
Attachment #611769 - Flags: review?(tterribe)
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).
yeah, you're right, it should be vpx.pc instead of libvpx.pc.
Attached patch patch, v2 (obsolete) — Splinter Review
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 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-
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.
Would you like keep the compilation test or not? Because it's not necessary for the version check now.
(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.
Attached patch patch, v3 (obsolete) — Splinter Review
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+
Thanks, this one address the comments.
Attachment #615704 - Flags: checkin?
Attachment #615655 - Attachment is obsolete: true
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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee73897136b
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/eee73897136b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: