Spec URL: http://dougsland.com/zbar_package/zbar.spec SRPM URL: http://dougsland.com/zbar_package/zbar-0.8-1.fc11.src.rpm Description: A layered barcode scanning and decoding library. Supports EAN, UPC, Code 128, Code 39 and Interleaved 2 of 5. Includes applications for decoding captured barcode images and using a video device (eg, webcam) as a barcode scanner.
Just some quick comments on your specfile: - Please take a look at https://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries - 'Requires: pkgconfig' is missing in the -devel package https://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files - https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries shows the right syntax for ldconf Is rpmlint not complaining about the length of the lines in %description?
Hello Fabian, Thanks for your review, here a new version based on your comments. md5sum: e1ba7b73625a3f0a48f0b84f353f3a95 zbar-0.8-1.fc11.src.rpm c1df24ddee24b1d1cf5e413bf2589408 zbar.spec URL to download: http://dougsland.com/zbar_package/zbar.spec http://dougsland.com/zbar_package/zbar-0.8-1.fc11.src.rpm Cheers, Douglas
Hi Douglas, Is this your first package? I'm unable to find you in packager group nor any other package review request from you. If yes, please go ahead http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account and put FE-NEEDSPONSOR into Blocks field. if no, what is your FAS username?
Hello Jan, > Is this your first package? Yes > I'm unable to find you in packager group nor any other package review request > from you. > If yes, please go ahead > http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account > and put FE-NEEDSPONSOR into Blocks field. > if no, what is your FAS username? Done! Thanks for your review! My FAS username is: dougsland Cheers, Douglas
spec/srpm seem 404...
Hello Mamoru, This morning dougsland.com was down, please try again: http://dougsland.com/zbar_package/zbar-0.8-1.fc11.src.rpm http://dougsland.com/zbar_package/zbar.spec I have build a new spec and src. cb596a48b5eb0fe5559a541ee3653f64 zbar-0.8-1.fc11.src.rpm b05e48c859c8ebf7fec40eaab33839ac zbar.spec Cheers, Douglas
Just a note: Thanks also to Itamar Reis Peixoto <itamar.br> for reviewing the package.
(In reply to comment #7) I don't have sponsor power, Mamoru Tasaka is the guy and have the power to sponsor you and review your package, I recommend you to use fedorapeople to upload your .spec and src.rpm files. look -> http://fedoraproject.org/wiki/Infrastructure/fedorapeople.org
zbar results from koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1495552 Cheers, Douglas
Initial comments: * License - As far as I checked the whole source code, the license tag should be "LGPLv2+". * Description - You should not repeat the same description already written in the main package in every subpackage. * About Requires/BuildRequires - Usually "BuildRequires: pkgconfig" is redundant. Any -devel package containing .pc pkgconfig files should have "Requires: pkgconfig" or "Requires: /usr/bin/pkgconfig" and for example gtk2-devel has this. - Please explain why you want to add version specific dependency for ImageMagick-c++. Currently Fedora supports F-10/11/12 and the lowest version of ImageMagick in these branches is 6.4.0.10, ref: https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires - (Related to the above "Explicit Requires"), "Requires: gtk2" for -gtk subpackage, and "Requires: qt > 4" for -qt subpackages are redundant (and should be removed). - Usually the dependency between packages rebuilt from the same srpm should be exact EVR (Epoch-Version-Release) specific ( for example -gtk should have "Requires: %{name} = %{version}-%{release}") ref: https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package - -pygtk subpackage should depend on -gtk subpackage --------------------------------------------------------------- $ ldd -r ./zbarpygtk.so 2>/dev/null | grep zbargtk libzbargtk.so.0 => not found --------------------------------------------------------------- - The following line has typo. --------------------------------------------------------------- Provides: libzbarqt-static = {%version}-%{release} --------------------------------------------------------------- (should be %{version}) (but for static archives see below) * Conditional BR - build.log shows: --------------------------------------------------------------- 161 checking for xmlto... no 189 checking for X11/extensions/Xvlib.h... no --------------------------------------------------------------- * Please check if "BuildRequires: xmlto" is not needed. * Please consider to add "BuildRequires: libXv-devel" * Timestamps - Please consider to use --------------------------------------------------------------- make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" --------------------------------------------------------------- to keep timestamps on installed files as much as possible. This method usually works for Makefiles generated from recent autotools. * %files https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries - Please explain why you want to ship static archives (i.e. *.a files). Fedora strongly discourages this. Expecially as this package provides shared libraries currently I cannot figure out why these .a files are needed, ref: - libtool .la files must be removed (see the URL above) - Please use %{_mandir} instead of %{_datadir}/man : https://fedoraproject.org/wiki/Packaging/RPMMacros - Usually the file "INSTALL" is for people who want to build/install the package by themselves and not needed for people using rpm: https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation - You don't have to install the document files already in main package also to every subpackage. - -devel subpackage should own %{_includedir}/zbar directory: https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership https://fedoraproject.org/wiki/Packaging:UnownedDirectories - For -python subpackage, please don't use "%{_libdir}/python*" but use %{python_sitearch}, see: https://fedoraproject.org/wiki/Packaging/Python#System_Architecture By the way please change the release number of your spec file every time you modify your package to avoid confusion.
Hello Mamoru, First of all, thanks for you review! New version base on your comments available at: http://dougsland.fedorapeople.org/packages/zbar/F11/v0.2/ diff-spec-between-v0.1-and-v0.2 25-Jul-2009 02:08 7.9K zbar-0.8-1.fc11.src.rpm 25-Jul-2009 02:06 472K zbar-0.8.tar.bz2 25-Jul-2009 00:50 469K zbar.spec 25-Jul-2009 01:14 4.6K Results from koji available at: http://koji.fedoraproject.org/koji/taskinfo?taskID=1505485 Please let me know if you have any additional comment. Cheers, Douglas
As I said in the previous comment, please change the release number of your spec file every time you modify the spec file to avoid confusion. Also please actually write in %changelog what was modified.
Hello Mamoru, > Comment #12 From Mamoru Tasaka (mtasaka.u-tokyo.ac.jp) 2009-07-25 > 13:26:59 EDT > > As I said in the previous comment, please change the release > number of your spec file every time you modify the spec file > to avoid confusion. Also please actually write in %changelog > what was modified. Sure, they are now fixed. New spec, srpm, source and diff are available at: http://dougsland.fedorapeople.org/packages/zbar/F11/v0.3/ Results from Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1516980 Thanks for your review, let me know if you have any additional comment. Cheers, Douglas
For -3: * SourceURL - Missed in the previous comment, however for sourceforge hosted source, please follow: https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net * Requires / BuildRequires - "BuildRequires: libX11-devel" is redundant because gtk2-devel requires this (and this is also in BuildRequires). https://fedoraproject.org/wiki/Packaging/Guidelines#Requires - Now "Requires: ImageMagick-c++" on the main package is redundant (and should be removed) as the dependencies for libraries automatically detected by rpmbuild should pull in this dependency: - And again "Requires: gtk2" on -gtk subpackage, and "Requires: qt >= 4" on -qt subpackage are redundant (and should be removed). - "Requires: %{name} = %{version}-%{release}" on -pygtk subpackage is redundant because -pygtk subpackage requires -gtk subpackage and -gtk subpackage requires main package. * Provides - "Provides: libzbar-static" on -devel subpackage must be removed because no static archives are now included (and same on other devel related subpackages) * Directory ownership issue - The %files entry --------------------------------------------------------- %files %{_includedir}/zbar --------------------------------------------------------- contains the directory %_includedir/zbar itself and all files/directories/etc under this directory (please check what $ rpm -ql zbar-devel returns), while --------------------------------------------------------- %files %dir %{_includedir}/zbar --------------------------------------------------------- contains the directory %_includedir/zbar only. * Macros in %changelog - In changelog, please use %% instead of % to keep macros from being expanded ($ rpm -q --changelog zbar shows what is actually occuring)
Hello Mamoru, Here new version based on your last comments: http://dougsland.fedorapeople.org/packages/zbar/F11/v0.4 diff-from-v0-3-to-v0-4 27-Jul-2009 18:50 5.3K zbar-0.8-4.fc10.src.rpm 27-Jul-2009 18:48 474K zbar-0.8.tar.bz2 25-Jul-2009 00:50 469K zbar.spec 27-Jul-2009 18:49 6.4K Let me know if you have any additional comment. Thanks again! Cheers, Douglas
For -4: * Very misc issues - SourceURL does not follow the following yet: https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net ( your SourceURL seems to work, too, however we now recommend to use the URL style written in the above URL ) * Scriptlets - Calling /sbin/ldconfig on %post{,un} is not needed for -devel subpackages. * Directory ownership issue - The directory %{python_sitearch} _itself_ is owned by python and zbar-pygtk should not own this directory. Now: ------------------------------------------------------------ NOTE: Before being sponsored: This package will be accepted with another few work. But before I accept this package, someone (I am a candidate) must sponsor you. Once you are sponsored, you have the right to review other submitters' review requests and approve the packages formally. For this reason, the person who want to be sponsored (like you) are required to "show that you have an understanding of the process and of the packaging guidelines" as is described on : http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored Usually there are two ways to show this. A. submit other review requests with enough quality. B. Do a "pre-review" of other person's review request (at the time you are not sponsored, you cannot do a formal review) When you have submitted a new review request or have pre-reviewed other person's review request, please write the bug number on this bug report so that I can check your comments or review request. Fedora package collection review requests which are waiting for someone to review can be checked on my wiki page: http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets (Check "No one is reviewing") Review guidelines are described mainly on: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines http://fedoraproject.org/wiki/Packaging/Guidelines
(In reply to comment #16) > B. Do a "pre-review" of other person's review request > (at the time you are not sponsored, you cannot do > a formal review) > pre-review welcome here -> https://bugzilla.redhat.com/show_bug.cgi?id=477979
Hello Mamoru, > For -4: > > * Very misc issues > - SourceURL does not follow the following yet: > https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net > ( your SourceURL seems to work, too, however we now recommend to > use the URL style written in the above URL ) > >* Scriptlets > - Calling /sbin/ldconfig on %post{,un} is not needed for -devel > subpackages. > > * Directory ownership issue > - The directory %{python_sitearch} _itself_ is owned by python > and zbar-pygtk should not own this directory. All changes requested are done. http://dougsland.fedorapeople.org/packages/zbar/F11/v0.5/ diff-from-v0_4_to_v0_5 28-Jul-2009 23:57 2.4K zbar-0.8-5.fc10.src.rpm 28-Jul-2009 23:45 475K zbar-0.8.tar.bz2 25-Jul-2009 00:50 469K zbar.spec 28-Jul-2009 23:45 6.5K Results from koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1555848 Thanks Douglas
Hello Mamoru, Pre-review available at: https://bugzilla.redhat.com/show_bug.cgi?id=497833 Cheers, Douglas
Hello Itamar, >Comment #17 From Itamar Reis Peixoto (itamar.br) 2009-07-28 > >pre-review welcome here -> > >https://bugzilla.redhat.com/show_bug.cgi?id=477979 Your package looks sane to my eyes (pre-review). Cheers, Douglas
Well, - for tuna review (bug 497833), if I review this ticket (currently I am not sure if I can make time to review tuna review formally...) I have something to comment on tuna review request, however I will accept your initial comments for pre-review of tuna. ------------------------------------------------------------------- This package (zbar) is APPROVED by mtasaka ------------------------------------------------------------------- Please follow the procedure written on: http://fedoraproject.org/wiki/PackageMaintainers/Join from "Install the Client Tools (Koji)". Now I am sponsoring you. If you want to import this package into Fedora 9/10, you also have to look at http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT (after once you rebuilt this package on koji Fedora rebuilding system). If you have questions, please ask me. Removing NEEDSPONSOR.
Hello Mamoru, Thanks attention and for your amazing review! Cheers, Douglas
New Package CVS Request ======================= Package Name: zbar Short Description: Bar code reader Owners: dougsland Branches: F-10 F-11 EL-5 InitialCC: dougsland
CVS done.
Thanks Jason!
imo the spec url could have been http://zbar.sourceforge.net/ which is actual project website.
Hello Juha, > imo the spec url could have been http://zbar.sourceforge.net/ which is actual > project website. Agreed, applied! Thanks Douglas
Now closing.