Bug 713443 - Review Request: yelp-tools - Create, manage, and publish documentation for Yelp
Summary: Review Request: yelp-tools - Create, manage, and publish documentation for Yelp
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2011-06-15 12:58 UTC by Zeeshan Ali
Modified: 2011-06-17 16:14 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2011-06-17 16:14:03 UTC
Type: ---
mclasen: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)

Description Zeeshan Ali 2011-06-15 12:58:23 UTC
Spec URL: http://zeenix.fedorapeople.org/yelp-tools.spec
SRPM URL: http://zeenix.fedorapeople.org/yelp-tools-3.1.4-1.fc16.src.rpm
yelp-tools is a collection of scripts and build utilities to help create,
manage, and publish documentation for Yelp and the web. Most of the heavy
lifting is done by packages like yelp-xsl and itstool. This package just
wraps things up in a developer-friendly way.

Comment 1 Matthias Clasen 2011-06-16 03:20:29 UTC
Quick initial comments, more thorough review to follow a bit later:

- builds fine in mock, thats great

- it produces an empty debuginfo package,  because there are no binaries in the main package that debuginfo could be stripped out. It looks to me that you probably want to make your package noarch, since it contains just things in /usr/share and scripts, which should not be arch-dependent. Making this a noarch package will turn off debuginfo as a side-effect. You do that by adding 

BuildArch: noarch 

somewhere in the header

- You have a 


in %install. That is no longer necessary.

- Typo in changelog: Initialia

Comment 2 Matthias Clasen 2011-06-16 03:48:26 UTC
going through https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Package name: ok
Spec file name: ok
Packaging guidelines: see above for the redundant rm -rf in %install. Otherwise ok
License: ok
License field: ok
License file: should include COPYING.GPL as well
Spec file language: ok
Spec file readable: yes
Upstream source: ok
ExcludeArch: ok
BuildRequires: ok
Locale handling: ok
ldconfig: ok
system libraries: ok
relocatable: ok
directory ownership: ok
duplicate files: ok
permissions: ok
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
headers: ok
static libraries: ok
shared libraries: ok
devel deps: ok
libtool archives: ok
gui apps: ok
file ownership: ok
utf8 filenames: ok

- remove rm -rf
- include COPYING.GPL
- make package noarch

I would also recommend making the file list more explicit; no reason not to list the three scripts explicitly, and you could also avoid the gratitious use of %{name} there. But that is just a recommendation, not a requirement.

Comment 3 Zeeshan Ali 2011-06-16 12:46:00 UTC
Thanks for the quick review. Just uploaded the new SRPM and SPEC to the same URLs. Let me know if I missed something.

Comment 4 Matthias Clasen 2011-06-16 16:02:56 UTC
Thanks, you've addressed all my points. Approved.

Comment 5 Zeeshan Ali 2011-06-16 16:46:55 UTC
New Package SCM Request
Package Name: yelp-tools
Short Description: Create, manage, and publish documentation for Yelp
Owners: zeenix
Branches: f16
InitialCC: mclasen

Comment 6 Gwyn Ciesla 2011-06-16 16:53:02 UTC
Git done (by process-git-requests).

No need to request f16 branch, f16==devel.

Comment 7 Matthias Clasen 2011-06-17 16:14:03 UTC
Build is done by now, so closing this out

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