Bug 484560 - Review Request: pydb - An expanded version of the Python debugger
Review Request: pydb - An expanded version of the Python debugger
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jon Levell
Fedora Extras Quality Assurance
http://bashdb.sourceforge.net/pydb
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-08 06:09 EST by Paulo Roma Cavalcanti
Modified: 2009-03-05 05:04 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-05 05:04:01 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paulo Roma Cavalcanti 2009-02-08 06:09:56 EST
Description of problem:

Spec URL: http://people.atrpms.net/~pcavalcanti/specs/pydb.spec

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


Extended Python Debugger is a more complete debugger for Python
than the stock pdb.py debugger. It supports a "restart" command,
non-interactive POSIX-like line tracing, command options, disassembly
of instructions, and stack traces that give better information for exec
statements. Stepping/nexting skips over method/function "defs". It tries
to follow gdb's command set unless there is good reason not to.

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:

pydb integrates with ddd, providing a graphical environment for
python debugging.
Comment 1 Jon Levell 2009-02-24 18:02:18 EST
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.
Comment 2 Michael Schwendt 2009-02-25 04:18:13 EST
* 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}
Comment 3 Paulo Roma Cavalcanti 2009-02-25 04:56:40 EST
(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.
Comment 4 Michael Schwendt 2009-02-25 05:04:20 EST
> > fontconfig
> 
> I do not remember why. Maybe some issue with emacs.

Hint: Add a comment in the spec file. ;-)
Comment 5 Paulo Roma Cavalcanti 2009-02-25 10:50:48 EST
(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.
Comment 6 Jon Levell 2009-02-25 15:45:40 EST
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
Comment 7 Paulo Roma Cavalcanti 2009-02-25 18:20:43 EST
(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.
Comment 8 Jon Levell 2009-02-26 17:17:42 EST
Ok... package approved
Comment 9 Paulo Roma Cavalcanti 2009-02-28 04:28:44 EST
New Package CVS Request
=======================
Package Name: pydb
Short Description: Extended Python Debugger
Owners: roma
Branches: F-9 F-10 F-11(devel)
Comment 10 Kevin Fenzi 2009-02-28 18:39:00 EST
cvs done.

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