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