[Buildroot] [PATCH 1/1] exiv2: new package

[Buildroot] [PATCH 1/1] exiv2: new package

[Nicolas Serafini]

Add support for Exiv2 library and utility to manage image metadata

Signed-off-by: Nicolas Serafini <nicolas.serafini@sensefly.com>
---
 package/Config.in        |  1 +
 package/exiv2/Config.in  | 33 +++++++++++++++++++++++++++++++++
 package/exiv2/exiv2.hash |  4 ++++
 package/exiv2/exiv2.mk   | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+)
 create mode 100644 package/exiv2/Config.in
 create mode 100644 package/exiv2/exiv2.hash
 create mode 100644 package/exiv2/exiv2.mk

diff --git a/package/Config.in b/package/Config.in
index 28cf703..6abf344 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -625,6 +625,7 @@ menu "Graphics"
 	source "package/adwaita-icon-theme/Config.in"
 	source "package/atk/Config.in"
 	source "package/cairo/Config.in"
+	source "package/exiv2/Config.in"
 	source "package/fltk/Config.in"
 	source "package/fontconfig/Config.in"
 	source "package/freetype/Config.in"
diff --git a/package/exiv2/Config.in b/package/exiv2/Config.in
new file mode 100644
index 0000000..d03447b
--- /dev/null
+++ b/package/exiv2/Config.in
@@ -0,0 +1,33 @@
+config BR2_PACKAGE_EXIV2
+	bool "exiv2"
+	help
+	  Exiv2 is a C++ library and a command line utility to manage image metadata.
+	  It provides fast and easy read and write access to the Exif,
+	  IPTC and XMP metadata of images in various formats.
+
+	  Exiv2 is available with GPLv2+ or commercial license.
+
+	  http://www.exiv2.org/
+
+if BR2_PACKAGE_EXIV2
+
+config BR2_PACKAGE_EXIV2_COMMERCIAL
+	bool "Enable commercial"
+	help
+	  Build the commercial version for closed source project.
+	  A commercial license request is needed.
+	  http://www.exiv2.org/download.html#license
+
+config BR2_PACKAGE_EXIV2_PNG
+	bool "PNG image support"
+	select BR2_PACKAGE_ZLIB
+	help
+	  Build with PNG image support
+
+config BR2_PACKAGE_EXIV2_XMP
+	bool "XMP support"
+	select BR2_PACKAGE_EXPAT
+	help
+	  Build with XMP support
+
+endif
diff --git a/package/exiv2/exiv2.hash b/package/exiv2/exiv2.hash
new file mode 100644
index 0000000..d4f8c60
--- /dev/null
+++ b/package/exiv2/exiv2.hash
@@ -0,0 +1,4 @@
+# From http://www.exiv2.org/download.html
+md5 b8a23dc56a98ede85c00718a97a8d6fc exiv2-0.24.tar.gz
+# Locally calculated
+sha256 f4a443e6c7fb9d9f5e787732f76969a64c72c4c04af69b10ed57f949c2dfef8e exiv2-0.24.tar.gz
diff --git a/package/exiv2/exiv2.mk b/package/exiv2/exiv2.mk
new file mode 100644
index 0000000..cbf5bcd
--- /dev/null
+++ b/package/exiv2/exiv2.mk
@@ -0,0 +1,32 @@
+################################################################################
+#
+# exiv2
+#
+################################################################################
+
+EXIV2_VERSION = 0.24
+EXIV2_SOURCE = exiv2-$(EXIV2_VERSION).tar.gz
+EXIV2_SITE = http://www.exiv2.org
+EXIV2_LICENSE = GPLv2+ or commercial
+EXIV2_LICENSE_FILES = COPYING
+EXIV2_INSTALL_STAGING = YES
+
+ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
+EXIV2_CONF_OPTS += --enable-commercial --disable-nls --disable-lensdata
+endif
+
+ifeq ($(BR2_PACKAGE_EXIV2_PNG),y)
+EXIV2_CONF_OPTS += --with-zlib=$(STAGING_DIR)
+EXIV2_DEPENDENCIES += zlib
+else
+EXIV2_CONF_OPTS += --without-zlib
+endif
+
+ifeq ($(BR2_PACKAGE_EXIV2_XMP),y)
+EXIV2_CONF_OPTS += --with-expat=$(STAGING_DIR)/usr/lib
+EXIV2_DEPENDENCIES += expat
+else
+EXIV2_CONF_OPTS += --disable-xmp
+endif
+
+$(eval $(autotools-package))
-- 
2.1.3

[Buildroot] [PATCH 1/1] exiv2: new package

[Thomas Petazzoni]

Dear Nicolas Serafini,

Thanks for your contribution. See a few comments below.

On Wed, 5 Nov 2014 15:30:29 +0100, Nicolas Serafini wrote:

> diff --git a/package/exiv2/Config.in b/package/exiv2/Config.in
> new file mode 100644
> index 0000000..d03447b
> --- /dev/null
> +++ b/package/exiv2/Config.in
> @@ -0,0 +1,33 @@
> +config BR2_PACKAGE_EXIV2
> +	bool "exiv2"
> +	help
> +	  Exiv2 is a C++ library and a command line utility to manage image metadata.

This line is a bit too long. Please wrap the help text so that it
doesn't go over 72 columns.

> diff --git a/package/exiv2/exiv2.mk b/package/exiv2/exiv2.mk
> new file mode 100644
> index 0000000..cbf5bcd
> --- /dev/null
> +++ b/package/exiv2/exiv2.mk
> @@ -0,0 +1,32 @@
> +################################################################################
> +#
> +# exiv2
> +#
> +################################################################################
> +
> +EXIV2_VERSION = 0.24
> +EXIV2_SOURCE = exiv2-$(EXIV2_VERSION).tar.gz

This line is not needed, as it is the default value.

> +EXIV2_SITE = http://www.exiv2.org
> +EXIV2_LICENSE = GPLv2+ or commercial
> +EXIV2_LICENSE_FILES = COPYING

You should probably make this conditional:

ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
EXIV2_LICENSE = commercial
else
EXIV2_LICENSE = GPLv2+
EXIV2_LICENSE_FILES = COPYING
endif

(Assuming COPYING contains the text of the GPLv2)

> +ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
> +EXIV2_CONF_OPTS += --enable-commercial --disable-nls --disable-lensdata
> +endif

Why --disable-nls here? It is already passed automatically by the
package infrastructure when locale support is not available.

Maybe a comment about why --disable-lensdata is passed when the
commercial license is used would be useful.

> +
> +ifeq ($(BR2_PACKAGE_EXIV2_PNG),y)
> +EXIV2_CONF_OPTS += --with-zlib=$(STAGING_DIR)
> +EXIV2_DEPENDENCIES += zlib
> +else
> +EXIV2_CONF_OPTS += --without-zlib
> +endif
> +
> +ifeq ($(BR2_PACKAGE_EXIV2_XMP),y)
> +EXIV2_CONF_OPTS += --with-expat=$(STAGING_DIR)/usr/lib

Maybe --enable-xmp here?

> +EXIV2_DEPENDENCIES += expat
> +else
> +EXIV2_CONF_OPTS += --disable-xmp
> +endif
> +
> +$(eval $(autotools-package))

Other than that, this patch looks good. Could you submit an updated
version that fixes the minor issues mentioned above?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] exiv2: new package

[Nicolas Serafini]

Hi Thomas,



Le 06. 11. 14 08:30, Thomas Petazzoni a ?crit :
> Dear Nicolas Serafini,
>
> Thanks for your contribution. See a few comments below.
>
> On Wed, 5 Nov 2014 15:30:29 +0100, Nicolas Serafini wrote:
>
>> diff --git a/package/exiv2/Config.in b/package/exiv2/Config.in
>> new file mode 100644
>> index 0000000..d03447b
>> --- /dev/null
>> +++ b/package/exiv2/Config.in
>> @@ -0,0 +1,33 @@
>> +config BR2_PACKAGE_EXIV2
>> +	bool "exiv2"
>> +	help
>> +	  Exiv2 is a C++ library and a command line utility to manage image metadata.
>
> This line is a bit too long. Please wrap the help text so that it
> doesn't go over 72 columns.

Ok I fix this.
I have not found the rule for maximum column in Config.in and *.mk files 
in the manual. Maybe we should add.

>
>> diff --git a/package/exiv2/exiv2.mk b/package/exiv2/exiv2.mk
>> new file mode 100644
>> index 0000000..cbf5bcd
>> --- /dev/null
>> +++ b/package/exiv2/exiv2.mk
>> @@ -0,0 +1,32 @@
>> +################################################################################
>> +#
>> +# exiv2
>> +#
>> +################################################################################
>> +
>> +EXIV2_VERSION = 0.24
>> +EXIV2_SOURCE = exiv2-$(EXIV2_VERSION).tar.gz
>
> This line is not needed, as it is the default value.


Ok, I didn't know that it was the default value. I fix this.

>
>> +EXIV2_SITE = http://www.exiv2.org
>> +EXIV2_LICENSE = GPLv2+ or commercial
>> +EXIV2_LICENSE_FILES = COPYING
>
> You should probably make this conditional:
>
> ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
> EXIV2_LICENSE = commercial
> else
> EXIV2_LICENSE = GPLv2+
> EXIV2_LICENSE_FILES = COPYING
> endif
>
> (Assuming COPYING contains the text of the GPLv2)

Ok I fix, good idea.

>
>> +ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
>> +EXIV2_CONF_OPTS += --enable-commercial --disable-nls --disable-lensdata
>> +endif
>
> Why --disable-nls here? It is already passed automatically by the
> package infrastructure when locale support is not available.
>
> Maybe a comment about why --disable-lensdata is passed when the
> commercial license is used would be useful.



The --disable-lensdata disable an included Nikon lens database for 
conversion to readable lens name and this database is free use only in 
non-commercial projects.

For the --disable-nls, I don't know exactly why it's requested in 
commercial version.
This is what is specified in the README file.


>
>> +
>> +ifeq ($(BR2_PACKAGE_EXIV2_PNG),y)
>> +EXIV2_CONF_OPTS += --with-zlib=$(STAGING_DIR)
>> +EXIV2_DEPENDENCIES += zlib
>> +else
>> +EXIV2_CONF_OPTS += --without-zlib
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_EXIV2_XMP),y)
>> +EXIV2_CONF_OPTS += --with-expat=$(STAGING_DIR)/usr/lib
>
> Maybe --enable-xmp here?

It works in both cases because XMP is automatically enabled if expat is 
found.

>
>> +EXIV2_DEPENDENCIES += expat
>> +else
>> +EXIV2_CONF_OPTS += --disable-xmp
>> +endif
>> +
>> +$(eval $(autotools-package))
>
> Other than that, this patch looks good. Could you submit an updated
> version that fixes the minor issues mentioned above?
>
> Thanks a lot!
>
> Thomas
>

I do the changes and I send the new patch

Thanks for the review.

Nicolas

[Buildroot] [PATCH 1/1] exiv2: new package

[Thomas Petazzoni]

Dear Nicolas Serafini,

On Thu, 6 Nov 2014 15:18:04 +0100, Nicolas Serafini wrote:

> Ok I fix this.
> I have not found the rule for maximum column in Config.in and *.mk files 
> in the manual. Maybe we should add.

Feel free to submit a patch :-)

> >> +ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
> >> +EXIV2_CONF_OPTS += --enable-commercial --disable-nls --disable-lensdata
> >> +endif
> >
> > Why --disable-nls here? It is already passed automatically by the
> > package infrastructure when locale support is not available.
> >
> > Maybe a comment about why --disable-lensdata is passed when the
> > commercial license is used would be useful.
> 
> The --disable-lensdata disable an included Nikon lens database for 
> conversion to readable lens name and this database is free use only in 
> non-commercial projects.

This is *bad*. It's not because you use the GPLv2 version of the
library that you're not making a commercial product. So please add a
separate Config.in option for this lensdata, disabled by default, with
a warning in the Config.in help text.

> For the --disable-nls, I don't know exactly why it's requested in 
> commercial version.
> This is what is specified in the README file.

Then please add a comment above the usage of --disable-nls to explain
that.

> >> +ifeq ($(BR2_PACKAGE_EXIV2_XMP),y)
> >> +EXIV2_CONF_OPTS += --with-expat=$(STAGING_DIR)/usr/lib
> >
> > Maybe --enable-xmp here?
> 
> It works in both cases because XMP is automatically enabled if expat is 
> found.

If --enable-xmp exists, then I would prefer to see it explicitly used.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] exiv2: new package

[Nicolas Serafini]

Dear THomas,

Ok I will fix all this.

I was a little to fast with the v2 of the Patch so do not take into account.

Thanks,

Nicolas

Le 06. 11. 14 16:01, Thomas Petazzoni a ?crit :
> Dear Nicolas Serafini,
>
> On Thu, 6 Nov 2014 15:18:04 +0100, Nicolas Serafini wrote:
>
>> Ok I fix this.
>> I have not found the rule for maximum column in Config.in and *.mk files
>> in the manual. Maybe we should add.
>
> Feel free to submit a patch :-)
>
>>>> +ifeq ($(BR2_PACKAGE_EXIV2_COMMERCIAL),y)
>>>> +EXIV2_CONF_OPTS += --enable-commercial --disable-nls --disable-lensdata
>>>> +endif
>>>
>>> Why --disable-nls here? It is already passed automatically by the
>>> package infrastructure when locale support is not available.
>>>
>>> Maybe a comment about why --disable-lensdata is passed when the
>>> commercial license is used would be useful.
>>
>> The --disable-lensdata disable an included Nikon lens database for
>> conversion to readable lens name and this database is free use only in
>> non-commercial projects.
>
> This is *bad*. It's not because you use the GPLv2 version of the
> library that you're not making a commercial product. So please add a
> separate Config.in option for this lensdata, disabled by default, with
> a warning in the Config.in help text.
>
>> For the --disable-nls, I don't know exactly why it's requested in
>> commercial version.
>> This is what is specified in the README file.
>
> Then please add a comment above the usage of --disable-nls to explain
> that.
>
>>>> +ifeq ($(BR2_PACKAGE_EXIV2_XMP),y)
>>>> +EXIV2_CONF_OPTS += --with-expat=$(STAGING_DIR)/usr/lib
>>>
>>> Maybe --enable-xmp here?
>>
>> It works in both cases because XMP is automatically enabled if expat is
>> found.
>
> If --enable-xmp exists, then I would prefer to see it explicitly used.
>
> Thanks,
>
> Thomas
>