Bug 437285 - Review Request: libgphoto2 - Library for accessing digital cameras
Review Request: libgphoto2 - Library for accessing digital cameras
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-13 07:56 EDT by Jindrich Novy
Modified: 2014-02-04 07:52 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-30 03:36:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jindrich Novy 2008-03-13 07:56:46 EDT
Spec URL: http://jnovy.fedorapeople.org/libgphoto2.spec
SRPM URL: http://jnovy.fedorapeople.org/libgphoto2-2.4.0-0.1.fc8.src.rpm
Description:

<description>
The gPhoto2 project is a universal, free application and library
framework that lets you download images from several different
digital camera models, including the newer models with USB
connections. Note that for some older camera models you must use
the old "gphoto" package.
</description>

Currently we have both gphoto2 as the user-level application and libgphoto2, which is a separate library packaged together in a single rpm package. This is odd since many applications need libgphoto2 what does not imply gphoto2 is needed.
Furthermore gphoto2 build process is hackish, where libgphoto2 is built and installed into the buildroot and finally gphoto2 is built on top of it. We should definitely avoid that and have two separate packages that BuildRequire each other instead.
Comment 1 Hans de Goede 2008-04-14 07:08:12 EDT
I'll review this, Jindrich, can you post URL's to a modified gphoto2 using this
separate libgphoto package too?
Comment 2 Jindrich Novy 2008-04-14 07:25:43 EDT
Sure, here we go:

Spec URL: http://jnovy.fedorapeople.org/gphoto2.spec
SRPM URL: http://jnovy.fedorapeople.org/gphoto2-2.4.0-7.fc8.src.rpm

Thanks!
Comment 3 Hans de Goede 2008-04-14 08:22:30 EDT
Full review done:

* This %define (luckily) does not seem to be used, please remove:
# Arches on which we need to prevent arch conflicts in gphoto2-config
%define multilib_arches %{ix86} ia64 ppc ppc64 s390 s390x x86_64

* I suppose this using 0.x scheme is only for the review, and you will jump to
  1%{?dist} after review?
Release: 0.1%{?dist}


* Source0: http://prdownloads.sourceforge.net/gphoto/libgphoto2-%{version}.tar.bz2

That is not the prefered form for a sf.net download url, that should be:

Source0: http://downloads.sourceforge.net/gphoto/libgphoto2-%{version}.tar.bz2


* Is this really needed? :
ExcludeArch: s390 s390x

I understand libgphoto is almost useless there, but it might be handy to still
have it so that applications which can use it don't have to have their specfiles
filled with %ifarch

* You should preserve the timestamps while converting the docs, so replace:
for i in AUTHORS COPYING; do
	cp ${i} ${i}.old
	iconv -f cp1250 -t utf-8 < ${i}.old > ${i}
	rm -f ${i}.old
done

with
for i in AUTHORS COPYING; do
	mv ${i} ${i}.old
	iconv -f cp1250 -t utf-8 < ${i}.old > ${i}
	touch -r ${i}.old ${i}
	rm -f ${i}.old
done

* License tag is wrong, the following source files are not LGPLv2+

GPLv2:
camlibs/adc65/adc65.c

GPLv2+:
camlibs/fuji/fuji.c
camlibs/minolta/dimagev/*
camlibs/mustek/*
camlibs/stv0680/*
libgphoto2/exif.c

LGPLv2:
camlibs/sipix/blink.c

So the correct license lines would be:
# GPLV2+ for the main lib (due to exif.c) and most plugins, some plugins GPLv2
License: GPLv2+ and GPLv2


* The descriptions need to be updated for the fact that this package now only
contains a lib and no longer the gphoto2 application.
Comment 4 Hans de Goede 2008-04-14 08:32:31 EDT
Note, these 2 remarks are valid for the separate gphoto2 itself too:

* This %define (luckily) does not seem to be used, please remove:
# Arches on which we need to prevent arch conflicts in gphoto2-config
%define multilib_arches %{ix86} ia64 ppc ppc64 s390 s390x x86_64

* Is this really needed? :
ExcludeArch: s390 s390x

Comment 5 Jindrich Novy 2008-04-16 12:39:31 EDT
(In reply to comment #3)
> Full review done:
> 
> * This %define (luckily) does not seem to be used, please remove:
> # Arches on which we need to prevent arch conflicts in gphoto2-config
> %define multilib_arches %{ix86} ia64 ppc ppc64 s390 s390x x86_64

Makes sense, removed.

> * I suppose this using 0.x scheme is only for the review, and you will jump to
>   1%{?dist} after review?
> Release: 0.1%{?dist}

Yup, the 0.x versioning is used just for review.

> 
> * Source0: http://prdownloads.sourceforge.net/gphoto/libgphoto2-%{version}.tar.bz2
> 
> That is not the prefered form for a sf.net download url, that should be:
> 
> Source0: http://downloads.sourceforge.net/gphoto/libgphoto2-%{version}.tar.bz2
> 

Updated.

> * Is this really needed? :
> ExcludeArch: s390 s390x
> 
> I understand libgphoto is almost useless there, but it might be handy to still
> have it so that applications which can use it don't have to have their specfiles
> filled with %ifarch

Ok, libgphoto2 is now compiled also for s390, s390x

> * You should preserve the timestamps while converting the docs, so replace:
> for i in AUTHORS COPYING; do
> 	cp ${i} ${i}.old
> 	iconv -f cp1250 -t utf-8 < ${i}.old > ${i}
> 	rm -f ${i}.old
> done
> 
> with
> for i in AUTHORS COPYING; do
> 	mv ${i} ${i}.old
> 	iconv -f cp1250 -t utf-8 < ${i}.old > ${i}
> 	touch -r ${i}.old ${i}
> 	rm -f ${i}.old
> done

Applied.

> * License tag is wrong, the following source files are not LGPLv2+
> 
> GPLv2:
> camlibs/adc65/adc65.c
> 
> GPLv2+:
> camlibs/fuji/fuji.c
> camlibs/minolta/dimagev/*
> camlibs/mustek/*
> camlibs/stv0680/*
> libgphoto2/exif.c
> 
> LGPLv2:
> camlibs/sipix/blink.c
> 
> So the correct license lines would be:
> # GPLV2+ for the main lib (due to exif.c) and most plugins, some plugins GPLv2
> License: GPLv2+ and GPLv2
> 

Fixed.

> * The descriptions need to be updated for the fact that this package now only
> contains a lib and no longer the gphoto2 application.
> 

I grabbed the description from the README file, hope it's sufficient.
Comment 6 Jindrich Novy 2008-04-16 12:41:27 EDT
(In reply to comment #4)
> Note, these 2 remarks are valid for the separate gphoto2 itself too:
> 
> * This %define (luckily) does not seem to be used, please remove:
> # Arches on which we need to prevent arch conflicts in gphoto2-config
> %define multilib_arches %{ix86} ia64 ppc ppc64 s390 s390x x86_64

Ok, I fixed it on the gphoto2 side as well. It really doesn't make sense any more :)

> * Is this really needed? :
> ExcludeArch: s390 s390x
> 

I'd keep it for gphoto2, Fedora is not built for s390(x) anyway so it doesn't
affect it and eases the Fedora -> RHEL move.
Comment 8 Hans de Goede 2008-04-16 14:30:13 EDT
All good now, approved!
Comment 9 Jindrich Novy 2008-04-16 15:01:30 EDT
Thanks for the review!
Comment 10 Jindrich Novy 2008-04-17 01:58:05 EDT
New Package CVS Request
=======================
Package Name: libgphoto2
Short Description: Library for accessing digital cameras
Owners: jnovy
Branches: 
InitialCC: 
Cvsextras Commits: yes
Comment 11 Kevin Fenzi 2008-04-17 12:53:22 EDT
cvs done.
Comment 12 Hans de Goede 2014-02-04 04:51:06 EST
Package Change Request
======================
Package Name: libgphoto2
Branches: f19
New Owners: twaugh jwrdegoede

libgphoto2 somehow has ended up in an orphaned state for f19, while it it still actively supported (and not orphaned for f20+), please unorphan it for f19 making twaugh and jwrdegoede the owners like they are for f20+.
Comment 13 Jon Ciesla 2014-02-04 07:52:09 EST
Complete.

Note You need to log in before you can comment on or make changes to this bug.