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