Configure libvpx with pkgconfig

RESOLVED FIXED in mozilla14

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Martin Stránský, Assigned: Martin Stránský)

Tracking

Trunk
mozilla14
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 611769 [details] [diff] [review]
patch

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

Patch for libvpx detection with pkgconfig
Attachment #611769 - Flags: review?(tterribe)

Comment 1

5 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

5 years ago
yeah, you're right, it should be vpx.pc instead of libvpx.pc.
(Assignee)

Comment 3

5 years ago
Created attachment 611799 [details] [diff] [review]
patch, v2

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-
(Assignee)

Comment 5

5 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

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 615655 [details] [diff] [review]
patch, v3

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

5 years ago
Created attachment 615704 [details] [diff] [review]
patch for check-in

Thanks, this one address the comments.
Attachment #615704 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.