Spec URL: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/psycopg2.spec SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/psycopg2-2.0.2-py2.4_1.src.rpm Description: psycopg is a PostgreSQL database adapter for the Python programming language (just like pygresql and popy.) It was written from scratch with the aim of being very small and fast, and stable as a rock. The main advantages of psycopg are that it supports the full Python DBAPI-2.0 and being thread safe at level 2.
I'm not experienced in reviewing packages and I'm not official reviewer, but first thing that came to my head is that you don't need to specify python version in release number. Packaging Naming Guidelines doesn't say anything about that. Also, python version will be assigned to package by python dependency. More important thing is wrong name of package. According to Package Naming Guidelines package' name should be python-psycopg2. Take a look on it: http://fedoraproject.org/wiki/Packaging/NamingGuidelines And the last thing: mock builds fail. It looks like adding dos2unix BuildRequires fixes this problem.
Hi, First of all: Thanks for the comments: (In reply to comment #1) > I'm not experienced in reviewing packages and I'm not official reviewer, but > first thing that came to my head is that you don't need to specify python > version in release number. Packaging Naming Guidelines doesn't say anything > about that. Also, python version will be assigned to package by python > dependency. Hmm. Ok. I removed the relevant part. > More important thing is wrong name of package. According to Package Naming > Guidelines package' name should be python-psycopg2. Take a look on it: > http://fedoraproject.org/wiki/Packaging/NamingGuidelines Great. That was a last-minute change I've applied before submitting. I reverted it. > And the last thing: mock builds fail. It looks like adding dos2unix > BuildRequires fixes this problem. Good catch. dos2unix was again a last-minute add to the spec. I've committed this change, too. I'll write URLS for the new SRPM and spec file link as a new comment. Regards, Devrim
New spec per comment above: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2.spec and new SRPM is: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2-2.0.2-2.src.rpm
You changed name of package, but you forgot to change name in subpackage doc. It is nice to use macros in it: Requires: %{name} = %{version}-%{release} Next thing of the doc subpackage is its group. According to Packaging Guidelines the Group name of doc subpackage should be Documentation, not Applications/Databases. From the main package you can delete python dependency, because rpm automatically adds it (I forgot about it, yesterday). The last thing is about doc subpackage again. I think it should be noarch, but then package won't compile well. Maybe, someone more experienced in reviewing would give you a good advice.
Hi, (In reply to comment #4) > You changed name of package, but you forgot to change name in subpackage doc. > It is nice to use macros in it: > Requires: %{name} = %{version}-%{release} Thanks for the prompt. I've applied the change. > Next thing of the doc subpackage is its group. According to Packaging > Guidelines the Group name of doc subpackage should be Documentation, not > Applications/Databases. Thanks, change applied. > From the main package you can delete python dependency, because rpm > automatically adds it (I forgot about it, yesterday). Yeah, that's what I noticed yesterday after reading Python Guideline. Thanks again, I've deleted it. > The last thing is about doc subpackage again. I think it should be > noarch, but then package won't compile well. Maybe, someone more experienced > in reviewing would give you a good advice. Per recent discussing in the list, I'm ignoring this. Regards,Devrim
The URL of spec did not change: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2.spec ... and the new URL for the new SRPM: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2-2.0.2-3.src.rpm Thanks. Devrim
One more thing: using of dos2unix is unnecessarily. Better use sed, it don't make new dependencies. Take a look on last section of it: http://fedoraproject.org/wiki/Packaging/CommonRpmlintIssues When you fix it, I think your package will be ready. But it's your first package, so you need a sponsor. Review another packages according to http://fedoraproject.org/wiki/Packaging/ReviewGuidelines and try to make another packages, so you'll bring sponsors attention. Good luck!
(In reply to comment #7) > One more thing: using of dos2unix is unnecessarily. Better use sed, it don't > make new dependencies. Take a look on last section of it: > http://fedoraproject.org/wiki/Packaging/CommonRpmlintIssues That was good to learn. I need the same thing for pgadmin3 community rpm... Thanks, applied. I did not change the URL for the spec and the SRPM; just re-uploaded them. > Good luck! Thanks. Regards, Devrim
I forgot about one another thing. You should add %{?dist} tag to Release number. It should look like: Release: 3%{?dist} For more information go here: http://fedoraproject.org/wiki/Packaging/DistTag
(In reply to comment #9) > You should add %{?dist} tag to Release number. It should look like: > Release: 3%{?dist} Thanks, done.
NEEDSWORK If you need some explanation of some of the blocker items, leave me a note here. I'll be back online tonight (~8 hours) MD5Sums: f7806d7141403b062a7353341bb393b5 python-psycopg2-2.0.2-3.src.rpm 6c056b261782c51f53ceccbf86c08749 psycopg2-2.0.2.tar.gz Blockers: * Source must have a full URL. * Package is licensed as GPL but should be GPL with exceptions (to allow OpenSSL and libpq [postgreSQL] linkage). * Why are you only installing the _psycopg.so file instead of the whole distribution? * The source package includes a Zope database adapter. Why not include it as a subpackage? * Package does not build on x86_64. The %install hardcodes the directory: lib.linux-i686-%{pyver} Why not use setup.py install? Cosmetic: * Since you already used "python" to extract the %{pyver} macro, there's no need to use %{pyver} in this line: python%{pyver} setup.py build Good: * Package is named according to the naming guidelines for python modules. * Spec name matches the package name. * Package includes a copy of the license. * The package is licensed with an Open source license. * Source matches upstream. * Source matches upstream GPG signature for: "Federico Di Gregorio <fog>" 06468DEB. * No locale files, therefore no need to run %find_lang. * No shared libraries for the dynamic linker to find so no ldconfig run necessary. * Package is not relocatable. * Package owns all directories it creates. * Permissions set correctly. * Package has a proper %clean. * Uses macros consistenly except for python%{pyver} note above. * Contains code, not content. * %doc files do not affect runtime. * No headers, pkgconfig files, or shared libraries so no -devel package. * Subpackages require the exact NEVR of the base package. * No .la files. * Not a GUI app so no desktop file. * Package builds on x86_64 Not yet checked: * rpmlint * Packaging Guidelines * Buildrequires * Requires * doc subpackage * Directory ownership.
Hi, (In reply to comment #11) > Blockers: > * Source must have a full URL. Fixed. > * Package is licensed as GPL but should be GPL with exceptions (to allow > OpenSSL and libpq [postgreSQL] linkage). I've changed it to LGPL. Is that ok? > * Why are you only installing the _psycopg.so file instead of the whole > distribution? My bad. Just a copy-paste error. I've highly modified spec file for this. > * The source package includes a Zope database adapter. Why not include it as > a subpackage? http://www.initd.org/tracker/psycopg/wiki/PsycopgTwoInstall This tells a little about the installation of Zope adapter; however I don't know where the "my_zope_instance" directory is > * Package does not build on x86_64. The %install hardcodes the directory: > lib.linux-i686-%{pyver} > Why not use setup.py install? Fixed per modifying the spec, as I wrote above. > Cosmetic: > * Since you already used "python" to extract the %{pyver} macro, there's no > need to use %{pyver} in this line: > python%{pyver} setup.py build Ok, done. > Good: <skipping> > Not yet checked: > * rpmlint I checked now, and it is clean. > * Packaging Guidelines > * Buildrequires I've removed mx and python-setuptools. mx is not needed in psycopg2. > * Requires I've removed mx from here, too. > * doc subpackage > * Directory ownership. I'll submit a new spec and srpm shortly. Regards, Devrim
Today psycopg2 2.0.3 was released, and here is a new spec and srpm with including new release and the fixes mentioned above: SPEC: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2.spec SRPM: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2-2.0.3-1.src.rpm Regards, Devrim
Fixed: * Source must have a full URL. * Now using setup.py install to install the whole module. Regressions: * The package is not licensed under the LGPL. The details are in the LICENSE file. The package is GPL with exceptions only for libpq and openssl. * The build is now broken on x86_64 because you've switched from the sitearch macro to the sitelib macro. Since psycopg builds and installs a compiled, shared object, not just python scripts and bytecode, the whole thing will install into the sitearch directory (/usr/lib64/ on x86_64). Reverting to the python_sitearch macro will solve this. Unresolved: * Zope database adapter. The Zope package in Extras creates the Products directory as: /var/lib/zope/Products Does that help?
Hi, (In reply to comment #14) > Regressions: > * The package is not licensed under the LGPL. The details are in the > LICENSE file. The package is GPL with exceptions only for libpq and openssl. Done. Please ignore the rpmlint warning about license. > * The build is now broken on x86_64 because you've switched from the sitearch > macro to the sitelib macro. Since psycopg builds and installs a compiled, > shared object, not just python scripts and bytecode, the whole thing will > install into the sitearch directory (/usr/lib64/ on x86_64). Reverting to > the python_sitearch macro will solve this. Ok. Could you please re-check it now? > Unresolved: > * Zope database adapter. The Zope package in Extras creates the Products > directory as: /var/lib/zope/Products Does that help? A lot. It now has a -zope package. I'll post spec and srpm URL as another comment... Regards, Devrim
New spec: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2.spec New srpm: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2-2.0.3-2.src.rpm Regards, Devrim
I've modified the spec a bit but did not bump of the version number. I just added a macro for zope adapter. The srpm and spec url is the same. Regards, Devrim
MD5Sums: f7806d7141403b062a7353341bb393b5 python-psycopg2-2.0.2-3.src.rpm 74219613b9f5b187f4e476264e2c966c psycopg2-2.0.3.tar.gz 7d692b049a1bcbc7c884eece65f98b9f python-psycopg2.spec All previous blockers have been fixed. Here's the new problems I've found: * %define ZPsycopgDAdir /var/lib/zope/Products/ZPsycopgDA/ hard codes /var. Rather than do that you should use %{_localstatedir}. * You need to own the ZPsycoDAdir in the %files section. * Since you're listing the Zope *.pyo's separate from the other files, you should ghost them just as you've ghosted the .pyo's in the main package: %ghost %{ZPsycopgDAdir}/*.pyo * The Zope subpackage should Require: zope or %{_localstatedir}/lib/zope * Using cp -p instead of just cp when installing the zope subpackage will preserve the file timestamps.
Hello, (In reply to comment #18) > All previous blockers have been fixed. Here's the new problems I've found: > * %define ZPsycopgDAdir /var/lib/zope/Products/ZPsycopgDA/ hard codes /var. > Rather than do that you should use %{_localstatedir}. > * You need to own the ZPsycoDAdir in the %files section. > * Since you're listing the Zope *.pyo's separate from the other files, you > should ghost them just as you've ghosted the .pyo's in the main package: > %ghost %{ZPsycopgDAdir}/*.pyo > * The Zope subpackage should Require: zope or %{_localstatedir}/lib/zope > * Using cp -p instead of just cp when installing the zope subpackage will > preserve the file timestamps. Thanks for the review. I fixed all. New SPEC: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2.spec New SRPM: http://developer.postgresql.org/~devrim/rpms/other/psycopg2/python-psycopg2-2.0.3-3.src.rpm Regards, Devrim
APPROVED dda9b98415609b18e331855af6cf09c7 python-psycopg2-2.0.3-3.src.rpm 74219613b9f5b187f4e476264e2c966c psycopg2-2.0.3.tar.gz 969e406141358bd286652e1a3bb858a9 python-psycopg2.spec All blockers fixed. Package builds in mock, installs and runs. rpmlint only complains about the License (GPL with exceptions) which is fine. This package is approved. Let's work on getting you approved this week so you can import this yourself. If we don't get that done, I can import the package and gateway your changes into cvs until you are sponsored but it's better if you can do that yourself.
Changed summary for tracking purposes.
Please leave the component of review tickets to "Package Review". Thanks.
Package Change Request ====================== Package Name: foobar New Branches: EL-4 EL-5
done