Bug 437285

I'll review this, Jindrich, can you post URL's to a modified gphoto2 using this
separate libgphoto package too?

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!

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.

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

(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.

(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.

New specs/srpms are here:

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

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

All good now, approved!

Thanks for the review!

New Package CVS Request
=======================
Package Name: libgphoto2
Short Description: Library for accessing digital cameras
Owners: jnovy
Branches: 
InitialCC: 
Cvsextras Commits: yes

cvs done.

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+.

Complete.