Last Comment Bug 741737 - Configure libvpx with pkgconfig
: Configure libvpx with pkgconfig
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Martin Stránský
:
Mentors:
Depends on: 722127
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 03:57 PDT by Martin Stránský
Modified: 2012-04-18 13:21 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.76 KB, patch)
2012-04-03 03:57 PDT, Martin Stránský
no flags Details | Diff | Review
patch, v2 (4.75 KB, patch)
2012-04-03 07:38 PDT, Martin Stránský
tterribe: review-
Details | Diff | Review
patch, v3 (4.42 KB, patch)
2012-04-17 04:10 PDT, Martin Stránský
khuey: review+
Details | Diff | Review
patch for check-in (4.38 KB, patch)
2012-04-17 07:27 PDT, Martin Stránský
no flags Details | Diff | Review

Description Martin Stránský 2012-04-03 03:57:09 PDT
Created attachment 611769 [details] [diff] [review]
patch

+++ This bug was initially created as a clone of Bug #722127 +++

Patch for libvpx detection with pkgconfig
Comment 1 Takanori MATSUURA 2012-04-03 06:22:21 PDT
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).
Comment 2 Martin Stránský 2012-04-03 07:25:53 PDT
yeah, you're right, it should be vpx.pc instead of libvpx.pc.
Comment 3 Martin Stránský 2012-04-03 07:38:46 PDT
Created attachment 611799 [details] [diff] [review]
patch, v2

Patch for vpx.pc (isntead of the previous for libvpx.pc).
Comment 4 Timothy B. Terriberry (:derf) 2012-04-03 10:58:33 PDT
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?
Comment 5 Martin Stránský 2012-04-05 07:12:04 PDT
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.
Comment 6 Martin Stránský 2012-04-05 08:44:11 PDT
Would you like keep the compilation test or not? Because it's not necessary for the version check now.
Comment 7 Timothy B. Terriberry (:derf) 2012-04-06 05:23:57 PDT
(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.
Comment 8 Martin Stránský 2012-04-17 04:10:09 PDT
Created attachment 615655 [details] [diff] [review]
patch, v3

Updated the version check and removed the build check.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-17 04:56:53 PDT
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?
Comment 10 Martin Stránský 2012-04-17 07:27:11 PDT
Created attachment 615704 [details] [diff] [review]
patch for check-in

Thanks, this one address the comments.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-04-17 16:52:58 PDT
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.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-04-17 16:58:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee73897136b
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-18 13:21:49 PDT
https://hg.mozilla.org/mozilla-central/rev/eee73897136b

Note You need to log in before you can comment on or make changes to this bug.