[v2,1/2] libmediaart: new package

Library tasked with managing, extracting and handling media art caches

https://github.com/GNOME/libmediaart

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
Changes v1 -> v2 (after review of Rahul Bedarkar):
 - Update version from 1.9.0 to 1.9.1
 - Update license from LGPLv2+ to LGPLv2.1+
 - Add a link to libmediaart github repository

 package/Config.in                    |  1 +
 package/libmediaart/Config.in        | 44 ++++++++++++++++++++++++++++++++++++
 package/libmediaart/libmediaart.hash |  2 ++
 package/libmediaart/libmediaart.mk   | 40 ++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+)
 create mode 100644 package/libmediaart/Config.in
 create mode 100644 package/libmediaart/libmediaart.hash
 create mode 100644 package/libmediaart/libmediaart.mk

Hello,

On Fri, 24 Mar 2017 21:38:50 +0100, Fabrice Fontaine wrote:

> +else ifeq ($(BR2_PACKAGE_MEDIAART_BACKEND_QT),y)
> +# qt5 needs c++11 (since qt-5.7)
> +ifeq ($(BR2_QT5_VERSION_LATEST),y)

Should be:

 BR2_PACKAGE_QT5_VERSION_LATEST

It has been changed in commit
https://git.buildroot.org/buildroot/commit/package/qt5/Config.in?id=49a33d3a229571e22ab783cbb1e5cace06ad7b99.

> +LIBMEDIAART_CONF_ENV += CXXFLAGS="$(TARGET_CXXFLAGS) -std=c++11"
> +endif
> +LIBMEDIAART_DEPENDENCIES += \
> +	$(if $(BR2_PACKAGE_QT),qt) \
> +	$(if $(BR2_PACKAGE_QT5),qt5base)
> +LIBMEDIAART_CONF_OPTS += \
> +	--disable-gdkpixbuf \
> +	--enable-qt
> +else ifeq ($(BR2_PACKAGE_MEDIAART_BACKEND_NONE),y)
> +LIBMEDIAART_CONF_OPTS += \
> +	--disable-gdkpixbuf \
> +	--disable-qt
> +endif

Are the backends really mutually exclusive?

Why don't we simply do:

ifeq ($(BR2_PACKAGE_QT)$(BR2_PACKAGE_QT5),y)
... enable QT backend ...
else
... disable QT backend
endif

ifeq ($(BR2_PACKAGE_GDK_PIXBUF),y)
... enable GDK pixbuf backend ...
else
... disable GDK pixbuf backend ...
endif

Thanks!

Thomas

Dear Thomas,

2017-03-24 21:57 GMT+01:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:

> Hello,
>
> On Fri, 24 Mar 2017 21:38:50 +0100, Fabrice Fontaine wrote:
>
> > +else ifeq ($(BR2_PACKAGE_MEDIAART_BACKEND_QT),y)
> > +# qt5 needs c++11 (since qt-5.7)
> > +ifeq ($(BR2_QT5_VERSION_LATEST),y)
>
> Should be:
>
>  BR2_PACKAGE_QT5_VERSION_LATEST
>
> It has been changed in commit
> https://git.buildroot.org/buildroot/commit/package/qt5/Config.in?id=
> 49a33d3a229571e22ab783cbb1e5cace06ad7b99.
>
> OK, I will update it.

> > +LIBMEDIAART_CONF_ENV += CXXFLAGS="$(TARGET_CXXFLAGS) -std=c++11"
> > +endif
> > +LIBMEDIAART_DEPENDENCIES += \
> > +     $(if $(BR2_PACKAGE_QT),qt) \
> > +     $(if $(BR2_PACKAGE_QT5),qt5base)
> > +LIBMEDIAART_CONF_OPTS += \
> > +     --disable-gdkpixbuf \
> > +     --enable-qt
> > +else ifeq ($(BR2_PACKAGE_MEDIAART_BACKEND_NONE),y)
> > +LIBMEDIAART_CONF_OPTS += \
> > +     --disable-gdkpixbuf \
> > +     --disable-qt
> > +endif
>
> Are the backends really mutually exclusive?
>
> Yes, they're mutually exclusive. If both are enabled, configure will fail
because of the followig lines in configure.ac:
if test "x$enable_qt" == "xyes" && test "x$enable_gdkpixbuf" == "xyes";
then
AC_MSG_ERROR([Can not enable both Qt and GdkPixbuf backends, please pick
one])
fi

> Why don't we simply do:
>
> ifeq ($(BR2_PACKAGE_QT)$(BR2_PACKAGE_QT5),y)
> ... enable QT backend ...
> else
> ... disable QT backend
> endif
>
> ifeq ($(BR2_PACKAGE_GDK_PIXBUF),y)
> ... enable GDK pixbuf backend ...
> else
> ... disable GDK pixbuf backend ...
> endif
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
Best Regards,

Fabrice

Hello,

On Sat, 25 Mar 2017 11:10:31 +0100, Fabrice Fontaine wrote:

> Yes, they're mutually exclusive. If both are enabled, configure will fail  
> because of the followig lines in configure.ac:
> if test "x$enable_qt" == "xyes" && test "x$enable_gdkpixbuf" == "xyes";
> then
> AC_MSG_ERROR([Can not enable both Qt and GdkPixbuf backends, please pick
> one])
> fi

OK, indeed. So we've got different solutions here:

 1. Use an explicit Config.in choice, like you did.

 2. Use automatic dependencies like I suggested, but enable only one
    backend at a time if both Qt and Gtk are enabled.

I guess the solution (1) you initially proposed is OK.

Thanks,

Thomas

Hello,

On Fri, 24 Mar 2017 21:38:50 +0100, Fabrice Fontaine wrote:
> Library tasked with managing, extracting and handling media art caches
> 
> https://github.com/GNOME/libmediaart
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>

I've applied the patch, but after doing a few changes.

First of all, you forgot to add yourself to the DEVELOPERS file for
this package, so I've added that. It's really important to do it, as it
ensures that you will be notified when there is a build failure for
this package.

> diff --git a/package/libmediaart/Config.in b/package/libmediaart/Config.in
> new file mode 100644
> index 0000000..5b6b6a7
> --- /dev/null
> +++ b/package/libmediaart/Config.in
> @@ -0,0 +1,44 @@
> +config BR2_PACKAGE_LIBMEDIAART
> +	bool "libmediaart"
> +	depends on BR2_USE_MMU # glib2
> +	depends on BR2_USE_WCHAR # glib2
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # glib2
> +	select BR2_PACKAGE_LIBGLIB2
> +	help
> +	  Library tasked with managing, extracting and handling media
> +	  art caches
> +
> +	  https://github.com/GNOME/libmediaart
> +
> +comment "libmediaart needs a toolchain w/ wchar, threads"
> +	depends on BR2_USE_MMU
> +	depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS

Having the comment here (between the definition of the main option and
the sub-options) breaks the indentation of the sub-options. So it
should either go at the top of the Config.in file, or at the very end.
I've opted for the former.

> +LIBMEDIAART_VERSION_MAJOR = 1.9
> +LIBMEDIAART_VERSION = $(LIBMEDIAART_VERSION_MAJOR).1
> +LIBMEDIAART_SOURCE = libmediaart-$(LIBMEDIAART_VERSION).tar.xz
> +LIBMEDIAART_SITE = \
> +	http://ftp.gnome.org/pub/gnome/sources/libmediaart/$(LIBMEDIAART_VERSION_MAJOR)
> +LIBMEDIAART_LICENSE = LGPLv2.1+
> +LIBMEDIAART_LICENSE_FILES = COPYING

This was not correct: COPYING is the text of GPLv2, not LGPLv2.1. I've
changed it to COPYING.LESSER, which really contains the text of
LGPLv2.1.

The only thing under GPLv2 seems to be the tests, which you disable, so
I've left this outside of the license information in the package.

Thanks!

Thomas