Bug 475132

Summary: Review Request: usbmon - Front-end for in-kernel usbmon
Product: [Fedora] Fedora Reporter: Pete Zaitcev <zaitcev>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, mail, notting
Target Milestone: ---Flags: kevin: fedora-review+
kevin: 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: 2008-12-26 20:15:16 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 Pete Zaitcev 2008-12-08 03:30:18 UTC
Spec URL: http://people.redhat.com/zaitcev/tmp/usbmon.spec
SRPM URL: http://people.redhat.com/zaitcev/tmp/usbmon-5.2-1.src.rpm

Description:
The usbmon program collects and prints a trace of USB transactions as they
occur between the USB core and HCDs. Analyzing the trace helps to debug the
kernel USB stack, device firmware, and applications.

This is the submitter's first package and a sponsor is needed.

Comment 1 Fabian Affolter 2008-12-08 09:35:19 UTC
Just some quick comments on your spec file

- 'Version: 4.05', the latest version is 4.06

- 'Group: File tools' is not a valid group.
  -> 'less /usr/share/doc/rpm-*/GROUPS' for a list with valid group entries

- 'Requires:  perl...' is missing, take a look at https://fedoraproject.org/wiki/Packaging/Perl#Versioned_MODULE_COMPAT__Requires

- '%defattr(-,root,root)' is normally '%defattr(-,root,root,-)'

- Your Changelog entry is missing the Release. See https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

Comment 2 Pete Zaitcev 2008-12-08 13:31:29 UTC
The comments above apparently belong to some different package.

Comment 3 Fabian Affolter 2008-12-08 14:02:08 UTC
Hmmm, yes the comments are for clamtk.  Sorry.

One for this package:  'Release:  1' should be 'Release:  1%{dist}'

Comment 4 Pete Zaitcev 2008-12-08 15:46:48 UTC
(In reply to comment #3)
> One for this package:  'Release:  1' should be 'Release:  1%{dist}'

Fixed
N.B. This changes the SRPM URL:
http://people.redhat.com/zaitcev/tmp/usbmon-5.2-1.fc11.src.rpm

Comment 5 Kevin Fenzi 2008-12-13 23:53:06 UTC
I'd be happy to review this and sponsor you... look for a full review in a bit.

Comment 6 Kevin Fenzi 2008-12-14 00:13:25 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License (GPLv2)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
cbba81a5b47b811dafd897cd7bd72dee  usbmon-5.2.tar.gz
cbba81a5b47b811dafd897cd7bd72dee  usbmon-5.2.tar.gz.orig


See below - Package needs ExcludeArch
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - No rpmlint output. 
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues: 

A few general comments, unrelated to the packaging and thus moot for the review, 
but I thought I would mention them: 

- You have your upstream url as your people.redhat.com page. 
Perhaps it would be good to use a fedorahosted.org site for this? 
That way you get bug tracking/mailing lists/vcs repo, etc. 
See: https://fedorahosted.org/web/new

- You might add a note about the license version to the .c file. 
No big deal since it's mentioned other places and is clearly your intent, 
but just to be paranoid. ;) 

Now, on to issues about the package: 

1. I have no idea on the ExcludeArch. Does s390 have usb? 
In any case it's not a blocker here as Fedora doesn't have s390 as a primary arch.
You might ask the s390 list?

I don't see any other blockers here, this is a very simple package, 
and is APPROVED. 

Go ahead and continue the process from
http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account
If you have any questions don't hesitate to contact me via bugzilla, email, 
or on irc (nirik on freenode).

Comment 7 Pete Zaitcev 2008-12-14 05:18:31 UTC
(In reply to comment #6)
> - You have your upstream url as your people.redhat.com page. 
> Perhaps it would be good to use a fedorahosted.org site for this? 
> That way you get bug tracking/mailing lists/vcs repo, etc. 
> See: https://fedorahosted.org/web/new

I hoped to get by, because thus far usbmon only received 3 patches
from external contributors. It's a very simple, even trivial tool.
But I'll keep this in mind, especially if they come out once they
see it packaged.

> - You might add a note about the license version to the .c file. 
> No big deal since it's mentioned other places and is clearly your intent, 
> but just to be paranoid. ;) 

How about I do it in 5.3, so as not to invalidate the approval of 5.2?

Comment 8 Kevin Fenzi 2008-12-14 06:40:44 UTC
>I hoped to get by, because thus far usbmon only received 3 patches
>from external contributors. It's a very simple, even trivial tool.
>But I'll keep this in mind, especially if they come out once they
>see it packaged.

Yeah, may be more overhead than needed now, but something to consider. 

> How about I do it in 5.3, so as not to invalidate the approval of 5.2?

If you want to post a updated spec/src.rpm, I would be happy to look it over. 
Or you can just import it with that change... thats fine as well.

Comment 9 Pete Zaitcev 2008-12-14 06:55:04 UTC
New Package CVS Request
=======================
Package Name: usbmon
Short Description: The front-end for usbmon
Owners: zaitcev
Branches: F-9 F-10
InitialCC: zaitcev

Comment 10 Kevin Fenzi 2008-12-14 07:00:45 UTC
cvs done.

Comment 11 Pete Zaitcev 2008-12-26 20:15:16 UTC
"yum install usbmon" works in Rawhide (10.90), closing as NEXTRELEASE.