Message ID | 1415197829-7086-1-git-send-email-nicolas.serafini@sensefly.com |
---|---|
State | Superseded |
Headers | show |
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
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
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
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 >
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))
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