[Bug 544384] Review Request: report - Incident reporting library

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=544384





--- Comment #3 from Gavin Romig-Koch <gavin at redhat.com>  2009-12-08 11:50:59 EDT ---

Michael, thank you very much for your review.  I have fixed what most of 
what you noted, includeing the Licence blocker, and explained why I did
change the rest below.   New spec file, SRPM, and tar ball at:


Spec URL: http://gavin.fedorapeople.org/report.spec
SRPM URL: http://gavin.fedorapeople.org/report-0.4-2.fc11.src.rpm
TAR  URL: http://gavin.fedorapeople.org/report-0.4.tar.gz



> > Name:           report
> 
> This ought to adhere to Fedora's Package Naming Guidelines for Python modules:
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29
> 
> 
> > Summary:        Incident reporting library
> 
> Actually, the terminology for this software would be "module" not "library".
> Mentioning that it's for Python would be good, too. Perhaps:
> 
> Summary: Python module for submitting reports to ticketing systems


I would like to stay with the 'report' name, or at least not make it 
Python specific.   While we are developing these initial versions in Python,
the plan is that as soon as the API seems solid enough, we are going to 
re-implement it in C/C++, and provided bindings from other interpreted 
languages (Python first, others as time permits and necessity requires).




> > Group:          System Environment/Base
> 
> Group "Development/Languages" sounds more appropriate, certainly for all
> (sub-)packages that don't include any ready-to-use executable. The RPM Group 
> is independent from the comps @base group.

Yes, 'base' is wrong.  Is 'System Environment/Libraries' better?  That's what
python-meh uses, and 'report' is much the same.



> > License:        GPLv2+
> 
>  That's a blocker. Nothing in the source tarball (except the .spec.in) confirms
> this licensing. Please include the GNU GPL license text, and as an added
> benefit follow its guidelines (consult its appendix) by adding brief GPL
> headers to the source files.

Oops.  Fixed.


> > %description plugin-catcut
> > Plugin reporter to catcut
> 
> Odd. Too brief. At least the description could try to explain what "catcut"
> means in this context.
> 
> > %files
> > ...
> > %attr(0664,root,root) %config(noreplace) /etc/catcut.config
> 
> Why is this package included in the base module package instead of the -catcut
subackage?
> 
> 
> > -rw-rw-r--  /etc/catcut.config
> 
> g+w  isn't really needed here. Nit-picky, I know. ;)  

Catcut is a ticketing interface that hasn't made (and may never make) it past
the experimental prototype stage.  I needed another ticketing system to test
report's plugin configurablity, and it was an easy one to use.  I should have,
and now have, pulled it out of this project.


> > Source0: report-0.4.tar.gz
> 
> https://fedoraproject.org/wiki/Packaging/SourceURL
> 

Yup. Fixed.



> > BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> 
> Specifying this is not necessary anymore with Fedora >= 10.
> 
> > %install
> > rm -rf $RPM_BUILD_ROOT
> 
> Empyting the buildroot is done by default with Fedora >= 10.
> 
> > %clean
> > rm -rf $RPM_BUILD_ROOT
> 
> There is a default %clean section with Fedora >= 10.

I want to get report into EPEL 5, once it's in Fedora in general, and EPEL 5
still needs these.  I should have noted this before.



> > Requires: report == 0.4
> 
> Consider yourself lucky that this worked. Prefer '=' instead of '=='.

Yup. Fixed. 


> > %dir %{python_sitelib}/report/alternatives/redhat_bugzilla
> > %{python_sitelib}/report/alternatives/redhat_bugzilla/*
> 
> Wherever you do the two-line  %dir plus '*' wildcard dance  you could simply
> use a single line instead, which achieves exactly the same thing and includes
> the directory and all its contents recursively:
> 
> %{python_sitelib}/report/alternatives/redhat_bugzilla/

Ah, thanks.  Fixed.




                                             -gavin...

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.