Bug 713443 - Review Request: yelp-tools - Create, manage, and publish documentation for Yelp
Review Request: yelp-tools - Create, manage, and publish documentation for Yelp
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 08:58 EDT by Zeeshan Ali
Modified: 2011-06-17 12:14 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-06-17 12:14:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Zeeshan Ali 2011-06-15 08:58:23 EDT
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-15 23:20:29 EDT
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-15 23:48:26 EDT
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 08:46:00 EDT
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 12:02:56 EDT
Thanks, you've addressed all my points. Approved.
Comment 5 Zeeshan Ali 2011-06-16 12:46:55 EDT
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 12:53:02 EDT
Git done (by process-git-requests).

No need to request f16 branch, f16==devel.
Comment 7 Matthias Clasen 2011-06-17 12:14:03 EDT
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.