Bug 199784 - Review Request: python-psycopg2 - A PostgreSQL database adapter for Python
Review Request: python-psycopg2 - A PostgreSQL database adapter for Python
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Toshio Kuratomi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-07-21 19:03 EDT by Devrim GUNDUZ
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-30 04:37:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
petersen: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Devrim GUNDUZ 2006-07-21 19:03:02 EDT
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.
Comment 1 Michał Bentkowski 2006-07-21 19:30:24 EDT
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.
Comment 2 Devrim GUNDUZ 2006-07-21 20:06:50 EDT
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
Comment 4 Michał Bentkowski 2006-07-22 06:57:42 EDT
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.
Comment 5 Devrim GUNDUZ 2006-07-22 08:48:36 EDT
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
Comment 6 Devrim GUNDUZ 2006-07-22 08:52:48 EDT
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
Comment 7 Michał Bentkowski 2006-07-22 09:38:23 EDT
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!
Comment 8 Devrim GUNDUZ 2006-07-22 11:00:02 EDT
(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
Comment 9 Michał Bentkowski 2006-07-22 11:10:28 EDT
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
Comment 10 Devrim GUNDUZ 2006-07-22 16:00:09 EDT
(In reply to comment #9)
> You should add %{?dist} tag to Release number. It should look like:
> Release:	3%{?dist}

Thanks, done. 
Comment 11 Toshio Kuratomi 2006-07-29 14:03:15 EDT
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@initd.org>" 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.
Comment 12 Devrim GUNDUZ 2006-07-31 07:11:06 EDT
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
Comment 13 Devrim GUNDUZ 2006-07-31 07:16:18 EDT
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
Comment 14 Toshio Kuratomi 2006-07-31 13:36:53 EDT
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?
Comment 15 Devrim GUNDUZ 2006-07-31 18:04:54 EDT
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
Comment 17 Devrim GUNDUZ 2006-07-31 19:42:03 EDT
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
Comment 18 Toshio Kuratomi 2006-08-05 15:58:06 EDT
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.
Comment 19 Devrim GUNDUZ 2006-08-06 01:43:21 EDT
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
Comment 20 Toshio Kuratomi 2006-08-06 16:36:37 EDT
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.
Comment 21 Christian Iseli 2007-01-02 18:18:37 EST
Changed summary for tracking purposes.
Comment 22 Christian Iseli 2007-01-02 19:08:54 EST
Please leave the component of review tickets to "Package Review".  Thanks.
Comment 23 Devrim GUNDUZ 2007-03-26 16:00:21 EDT
Package Change Request
======================
Package Name: foobar
New Branches: EL-4 EL-5
Comment 24 Jens Petersen 2007-03-27 08:25:09 EDT
done

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