Bug 713443

Summary:	Review Request: yelp-tools - Create, manage, and publish documentation for Yelp
Product:	[Fedora] Fedora	Reporter:	Zeeshan Ali <zeenix>
Component:	Package Review	Assignee:	Matthias Clasen <mclasen>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	fedora-package-review, mclasen, notting, tbzatek
Target Milestone:	---	Flags:	mclasen: fedora-review+ gwync: fedora-cvs+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2011-06-17 16:14:03 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

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
Description:
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 

rm -rf $RPM_BUILD_ROOT

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

Summary:
- 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