Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: report - Incident reporting library
https://bugzilla.redhat.com/show_bug.cgi?id=544384
Summary: Review Request: report - Incident reporting library Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: gavin@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Target Release: ---
Spec URL: http://gavin.fedorapeople.org/report.spec SRPM URL: http://gavin.fedorapeople.org/report-0.4-1.fc11.src.rpm Description:
A generic problem/bug/incident/error reporting library, that can be configured to deliver a report to a variety of different ticketing systems.
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 #1 from Gavin Romig-Koch gavin@redhat.com 2009-12-04 14:23:13 EDT --- $ rpmlint report.spec SRPMS/report-0.4-1.fc11.src.rpm RPMS/noarch/report-*.rpm report-gtk.noarch: W: no-documentation report-plugin-bugzilla.noarch: W: no-documentation report-plugin-catcut.noarch: W: no-documentation 5 packages and 1 specfiles checked; 0 errors, 3 warnings. $
The main package has minimal doc, the sub-packages don't have separate documentation.
The intent is that the main 'report' package would be part of @base, as would the subpackage 'report-plugin-bugzilla'. The 'report-gtk' subpackage would go into the same group as all the other X/gtk/gnome packages. The 'report-plugin-catcut' would not be install by default at all.
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
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mschwendt@gmail.com
--- Comment #2 from Michael Schwendt mschwendt@gmail.com 2009-12-05 06:45:24 EDT ---
Name: report
This ought to adhere to Fedora's Package Naming Guidelines for Python modules:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28...
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
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.
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.
%description plugin-catcut Plugin reporter to catcut
Odd. Too brief. At least the description could try to explain what "catcut" means in this context.
Source0: report-0.4.tar.gz
https://fedoraproject.org/wiki/Packaging/SourceURL
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.
Requires: report == 0.4
Consider yourself lucky that this worked. Prefer '=' instead of '=='.
%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?
%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/
-rw-rw-r-- /etc/catcut.config
g+w isn't really needed here. Nit-picky, I know. ;)
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@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_.28...
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
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...
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
Dan Horák dan@danny.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |dan@danny.cz Flag| |fedora-review?
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 #4 from Dan Horák dan@danny.cz 2009-12-17 05:28:07 EDT --- formal review is here, see the notes below:
OK source files match upstream: 70ab9e22d9f21e03c0e43357072ecea8ed55ddab report-0.4.tar.gz OK* package meets naming and versioning guidelines. OK* specfile is properly named, is cleanly written and uses macros consistently. OK dist tag is present. OK license field matches the actual license. OK license is open source-compatible (GPLv2+). License text not included upstream. OK latest version is being packaged. BAD BuildRequires are proper. N/A compiler flags are appropriate. OK %clean is present. OK package builds in mock (Rawhide/x86_64). N/A debuginfo package looks complete. OK* rpmlint is silent. BAD final provides and requires look sane. N/A %check is present and all tests pass. OK no shared libraries are added to the regular linker search paths. OK owns the directories it creates. OK doesn't own any directories it shouldn't. OK no duplicates in %files. OK file permissions are appropriate. OK no scriptlets present. OK code, not content. OK documentation is small, so no -docs subpackage is necessary. OK %docs are not necessary for the proper functioning of the package. OK no headers. OK no pkgconfig files. OK no libtool .la droppings. OK not a GUI app.
- I accept the reasoning for the package name from comment #3, but it can be useful for a straightforward upgrade path (after the python bindings are created) to add now Provide: python-report = %{version}-%{release} into the main package - the usual form of spec file contains the definition of subpackages directly after the main package and before the %prep section, the %files sections for the sub-packages are placed after the main %files section - you should omit the BuildArch and BuildRequires in the sub-packages, they are inherited from the main package - you should use Requires: %{name} = %{version}-%{release} in the sub-packages instead of just the hardcoded version - please include the license text in the upstream source archive and then as %doc in the package - you can use fedorahosted facility to publish the source archive - rpmlint complains a bit report.noarch: W: incoherent-version-in-changelog 0.4-1 ['0.4-2.fc13', '0.4-2'] => looks as an omission report-gtk.noarch: W: no-documentation report-plugin-bugzilla.noarch: W: no-documentation => can be ignored
Thanks goes to Michael for his almost complete review making my work much easier.
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 #5 from Gavin Romig-Koch gavin@redhat.com 2009-12-17 11:07:54 EDT --- (In reply to comment #4)
Thank you very much Dan.
I've fixed all the issues you noted. Updated:
Spec URL: https://fedorahosted.org/released/report/report.spec SRPM URL: https://fedorahosted.org/released/report/report-0.4-3.fc11.src.rpm TAR URL: https://fedorahosted.org/released/report/report-0.4.tar.gz
-gavin...
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
Dan Horák dan@danny.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #6 from Dan Horák dan@danny.cz 2009-12-18 04:25:32 EDT --- Thanks for update, all issues are fixed now, the package is APPROVED.
Just a little remark at the end - as an upstream author please never publish an updated source archive under the same filename, it breaks the automatic checks between sources stored in Fedora and sources defined by the Source tag.
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
Gavin Romig-Koch gavin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #7 from Gavin Romig-Koch gavin@redhat.com 2009-12-28 13:08:03 EDT --- New Package CVS Request ======================= Package Name: report Short Description: Incident reporting library Owners: gavin Branches: F-11 F-12 EL-5 InitialCC:
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
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #8 from Kevin Fenzi kevin@tummy.com 2009-12-28 21:51:55 EDT --- cvs done.
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 #9 from Gavin Romig-Koch gavin@redhat.com 2009-12-31 10:22:21 EDT --- Thanks everyone for your help and support.
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
Gavin Romig-Koch gavin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla