[Buildroot] [PATCH 1/1] libgphoto2: Add libgphoto2 package

Mon Apr 10 10:20:52 UTC 2017

 Hi Kevin,

 Some more feedback on your patch...

 A better summary line would be

libgphoto2: new package

On 06-04-17 10:33, Kevin JOLY wrote:
> Add support for libgphoto2 core library designed to allow access to digital camera.

 The commit message text should be wrapped at 72 columns.

> 
> Signed-off-by: Kevin JOLY <kevin.joly at sensefly.com>

 Since Romain also worked on this package, it's nice to add

Cc: Romain Naour <romain.naour at gmail.com>

This makes sure that:
- Romain will be in Cc when you use git send-email;
- In the git history, it will be visible that Romain was involved.

[snip]
> diff --git a/package/libgphoto2/Config.in b/package/libgphoto2/Config.in
> new file mode 100644
> index 0000000..1626667
> --- /dev/null
> +++ b/package/libgphoto2/Config.in
> @@ -0,0 +1,11 @@
> +config BR2_PACKAGE_LIBGPHOTO2
> +	select BR2_PACKAGE_LIBTOOL
> +	select BR2_PACKAGE_LIBUSB
> +	select BR2_PACKAGE_LIBUSB_COMPAT

 Is libusb-compat really needed? It seems to be only used if libusb1 isn't
found. Also, as far as I can see in the configure script, this is optional (you
just loose most of the ports).

> +	select BR2_PACKAGE_LIBXML2

 In your reply to v1, you mentioned that br-arm-full-nothread failed. That seems
to indicate that it should depend on BR2_TOOLCHAIN_HAS_THREADS (with the
appropriate comment in case of !BR2_TOOLCHAIN_HAS_THREADS). But anyway, this
dependency is already needed for libusb. So if the package itself doesn't
require threads (just grep for "pthread", and check whether or not it's escaped
by some #ifdef), you should say:

	depends on BR2_TOOLCHAIN_HAS_THREADS # libusb

> +	bool "libgphoto2"
> +	help
> +	  libgphoto2 is the core library designed to allow access to
> +	  digital camera by external programs.
> +
> +	  http://gphoto.org/

 For libgphoto2 itself:

http://gphoto.org/proj/libgphoto2/

> diff --git a/package/libgphoto2/libgphoto2.hash b/package/libgphoto2/libgphoto2.hash
> new file mode 100644
> index 0000000..2e659ab
> --- /dev/null
> +++ b/package/libgphoto2/libgphoto2.hash
> @@ -0,0 +1,6 @@
> +# https://sourceforge.net/projects/gphoto/files/libgphoto/2.5.12/
> +md5 a5999acc204c31515a6ec8e517d2cd91 libgphoto2-2.5.12.tar.bz2
> +sha1 4ded403b87d46ad49ba88b22b9410789ed3cef10 libgphoto2-2.5.12.tar.bz2
> +
> +# Locally calculated hash
> +sha256 b9bb28990fde45ac385e4851a07dbad2e1250404b535b0a3a3b898bb431e4e2e	libgphoto2-2.5.12.tar.bz2
> diff --git a/package/libgphoto2/libgphoto2.mk b/package/libgphoto2/libgphoto2.mk
> new file mode 100644
> index 0000000..354c29a
> --- /dev/null
> +++ b/package/libgphoto2/libgphoto2.mk
> @@ -0,0 +1,29 @@
> +################################################################################
> +#
> +# libgphoto2
> +#
> +################################################################################
> +
> +LIBGPHOTO2_VERSION = 2.5.12

 There's a 2.5.13 out now.

> +LIBGPHOTO2_SOURCE = libgphoto2-$(LIBGPHOTO2_VERSION).tar.bz2
> +LIBGPHOTO2_SITE = https://sourceforge.net/projects/gphoto/files/libgphoto/$(LIBGPHOTO2_VERSION)

 The website itself points to github, and the repository is maintained on github
(no commits in sourceforge since 2015), so I'd tend to say that you should use
the github download. However, the github download is just the autogenerated one
which doesn't contain a configure script, so indeed it's better to use
sourceforge. Still, it's better to add a comment about that:

# Project is maintained on github but github tarball doesn't have configure

> +LIBGPHOTO2_INSTALL_STAGING = YES
> +
> +LIBGPHOTO2_DEPENDENCIES = libxml2 libusb libusb-compat libtool host-pkgconf

 It looks like there are some configure options you may want to pass explicitly,
like --disable-rpm. The package also installs a lot of stuff in non-standard
locations, it would be good to check if the defaults are appropriate. Also take
a look at README.packaging.

> +
> +ifeq ($(BR2_PACKAGE_LIBEXIF),y)
> +LIBGPHOTO2_DEPENDENCIES += libexif

> +endif
> +
> +ifeq ($(BR2_PACKAGE_LIBJPEG),y)
> +LIBGPHOTO2_DEPENDENCIES += libjpeg

 Please add an explicit
LIBGPHOTO2_CONF_OPTS += --with-jpeg
else
LIBGPHOTO2_CONF_OPTS += --without-jpeg

 Ideally also for exif and gd, but it seems there are no options for those.

> +endif
> +
> +ifeq ($(BR2_PACKAGE_GD),y)
> +LIBGPHOTO2_DEPENDENCIES += gd
> +endif
> +
> +LIBGPHOTO2_LICENSE = LGPLv2.1+

 We switched completely to SPDX license strings, so LGPL-2.1+

> +LIBGPHOTO2_LICENSE_FILES = COPYING

 We typically put the license info near the top, just above or below the initial
DEPENDENCIES.

 It looks like some of the camlibs are actually GPL-2.0+ or LGPL-3+, and some
are LGPL-2.0 only. So the complete string should be:

LIBGPHOTO2_LICENSE = LGPLv2.1+, GPL-2.0 (adc65), GPL-2.0+ (some camlibs), \
	LGPL-2.0 (sipix), LGPL-3.0+ (pentax), BSD-3-Clause (ax203/tinyjpeg).

I couldn't find any license files for all the other licenses, so add a comment

# No license files for other licenses

 It would be good to double-check all of the above...

 Regards,
 Arnout

> +
> +$(eval $(autotools-package))
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF