Bug 484560

This is just an informal review:

1) Using rpmlint on your spec file shows that for the main package you have omitted the %defattr section. It should be 
%defattr(-,root,root,-)

2) For the emacs subpackage, the defattr line should also be changed to the above.

3) The license tag should be GPLv2+ as the copyright headers allow you to use the code under a later version.

4) building the package with mock and running rpmlint on the resulting rpms gives warnings:
rpmlint -i emacs-pydb-1.25-3.fc10.noarch.rpm
emacs-pydb.noarch: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include documentation files.

and 

rpmlint -i pydb-1.25-3.fc10.noarch.rpm
pydb.noarch: W: symlink-should-be-relative /usr/bin/pydb /usr/lib/python2.5/site-packages/pydb/pydb.py
Absolute symlinks are problematic eg. when working with chroot environments.

The first is border-line:
https://fedoraproject.org/wiki/PackageMaintainers/Common_Rpmlint_Issues#no-documentation
but the COPYING file should probably be included in at least one of the rpms

The second can be fixed by following an example like:
http://cvs.fedora.redhat.com/viewvc//rpms/python-kid/devel/python-kid.spec?view=markup


Other than the above, the rest looks fine.

* Preferably bump "Release" prior to offering new src.rpm builds. That's helpful when using rpmdev-diff. Here the 1.25-3.fc10 has been modified silently.


* "Group" could be "Development/Debuggers"


* "License: GPLv2+" and the source files mention GPL 2 or later, but file "COPYING" is the GPL 3. Can you get upstream to clarify this?


* Instead of

  %{_mandir}/man1/%{name}.1.gz

prefer

  %{_mandir}/man1/%{name}.1*

since the manual pages get compressed automatically, and the compression method and file extension may change.


* Why "BuildRequires: fontconfig"?


* It's good packaging-practice to run a test-suite target, if available, and provided that it is not known to be broken:

--- pydb.spec.orig      2009-02-25 00:37:38.000000000 +0100
+++ pydb.spec   2009-02-25 09:46:13.000000000 +0100
@@ -44,6 +44,9 @@
 make install DESTDIR=%{buildroot}
 ln -sf ../..%{python_sitelib}/%{name}/%{name}.py %{buildroot}%{_bindir}/%{name}
 
+%check
+make check
+
 %clean
 rm -rf %{buildroot}

(In reply to comment #2)
> * Preferably bump "Release" prior to offering new src.rpm builds. That's
> helpful when using rpmdev-diff. Here the 1.25-3.fc10 has been modified
> silently.
> 

Yes. You were faster than me. The new src.rpm is:

http://people.atrpms.net/~pcavalcanti/srpms/pydb-1.25-4.fc10.src.rpm

> 
> * "Group" could be "Development/Debuggers"

Done.

> 
> 
> * "License: GPLv2+" and the source files mention GPL 2 or later, but file
> "COPYING" is the GPL 3. Can you get upstream to clarify this?
> 
> 
> * Instead of
> 
>   %{_mandir}/man1/%{name}.1.gz
> 
> prefer
> 
>   %{_mandir}/man1/%{name}.1*

Done.

> 
> since the manual pages get compressed automatically, and the compression method
> and file extension may change.
> 
> 
> * Why "BuildRequires: fontconfig"?

I do not remember why. Maybe some issue with emacs. I'll go check.

> 
> 
> * It's good packaging-practice to run a test-suite target, if available, and
> provided that it is not known to be broken:
> 
>

Included.


I also had already included all Jon Levell's observations.

Thanks to both of you.

> > fontconfig
> 
> I do not remember why. Maybe some issue with emacs.

Hint: Add a comment in the spec file. ;-)

(In reply to comment #4)
> > > fontconfig
> > 
> > I do not remember why. Maybe some issue with emacs.
> 
> Hint: Add a comment in the spec file. ;-)

fontconfig does not seem to be necessary, indeed.

I removed the BR, and updated the spec and .src.rpm, but
did not change the release (it is a very small change).

I am also submitting bashdb, and I would appreciate if both of you
could take a look at it.

Thanks.

Personally, I would have put the BR change in the changelog and bumped the release. Aside from that, I can't see any new issues - I'll go take a look at bashdb

(In reply to comment #6)
> Personally, I would have put the BR change in the changelog and bumped the
> release. Aside from that, I can't see any new issues - I'll go take a look at
> bashdb

I registered the BR change, but I kept the release. I do not like
bumping releases when the changes are done in the same day.

Bashdb has already been approved, but thanks anyway.

Ok... package approved

New Package CVS Request
=======================
Package Name: pydb
Short Description: Extended Python Debugger
Owners: roma
Branches: F-9 F-10 F-11(devel)

cvs done.