Bug 617340

Summary: Review Request: throttle - copy stdin to stdout at the specified speed (or lower)
Product: [Fedora] Fedora Reporter: François Cami <fdc>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: drjohnson1, fedora-package-review, notting, pahan, tcallawa
Target Milestone: ---Flags: kevin: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: throttle-1.2-4.fc15 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-07-12 05:24:18 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 François Cami 2010-07-22 19:30:56 UTC
Spec URL: http://fcami.fedorapeople.org/srpms/throttle.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/throttle-1.2-2.fc13.src.rpm
Description: http://fcami.fedorapeople.org/srpms/throttle.spec

Throttle copies the standard input to the standard output while limiting bandwidth to the specified maximum.
For instance, to avoid overloading a system while using dd to create a file on the hard drive, you would run:
$ dd if=/dev/zero | throttle -m 20 | dd of=somefile bs=4096 count=100000

Please note that I need a sponsor as I am not a packager yet.

Comment 1 François Cami 2010-07-24 19:42:56 UTC
First review: https://bugzilla.redhat.com/show_bug.cgi?id=575529

Comment 3 d. johnson 2010-07-25 19:11:54 UTC
Please note, I am not an an official packager.

- Package meets naming and packaging guidelines
- Spec file matches base package name.
- Spec has consistent macro usage.
- Meets Packaging Guidelines.
- License
- License field in spec matches
- License file included in package
- Spec in American English
- Spec is legible.
- Sources match upstream md5sum:

- Package does not need ExcludeArch
- BuildRequires correct
- Spec handles locales/find_lang (none found in package)
- Package has %defattr and permissions on files is good.
- Package has a correct %clean section.
- Package has correct buildroot
- Package is code or permissible content.
- Doc subpackage not needed/used.
- Packages %doc files don't affect runtime.

- Package is a not a GUI app

- Package compiles and builds on at least one arch.
- Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.
- No rpmlint output.
- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done


SHOULD Items:

- Should build in mock.  (Does.)
- Should build on all supported archs (Builds on both i686 and x86_64)
- Should function as described. (Does)
- Should have sane scriptlets. (None)
- Should have subpackages require base package with fully versioned depend. (None)
- Should have dist tag (Does)
- Should package latest version (Does)
- check for outstanding bugs on package. (For core merge reviews)

% rpmlint throttle-*1.2-3.fc1*.rpm 
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 4 Kevin Fenzi 2010-07-28 18:28:27 UTC
I'll look at doing a review here soon. ;) 

If you have another package to submit also that would be good, or can do a few more 'pre-reviews'. 

One quick comment on this spec: 

Can you avoid using %makeinstall? See: 
https://fedoraproject.org/wiki/PackagingDrafts/MakeInstall
(or the non draft section in the main guidelines)

Comment 5 François Cami 2010-07-28 18:49:10 UTC
Thank you Kevin.

Updated Spec: http://fcami.fedorapeople.org/srpms/throttle.spec
Updated SRPM: http://fcami.fedorapeople.org/srpms/throttle-1.2-4.fc13.src.rpm
I've switched from %makeinstall to "make DESTDIR=%{buildroot} install".

Another package should be up tomorrow.

Comment 6 François Cami 2010-08-13 22:40:21 UTC
Another package review request:
https://bugzilla.redhat.com/show_bug.cgi?id=624179

Comment 7 François Cami 2010-08-13 23:29:32 UTC
Another package review request:
https://bugzilla.redhat.com/show_bug.cgi?id=624182

Comment 8 Kevin Fenzi 2010-08-14 18:40:08 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. 
See below - License
See below - 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:
bb8abf5a9a63ed7d80951f056543a88c  throttle-1.2.tar.gz
bb8abf5a9a63ed7d80951f056543a88c  ../rpm/throttle/throttle-1.2.tar.gz

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 - Package obey's FHS standard (except for 2 exceptions)
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 function as described. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. The license seems to be GPLv2, but throttle.c has: 
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License.

It looks like they might have meant, 'or any later version' there, or 
messed up cutting that off. Could you ask upstream to clarify if it's ONLY 
GPLv2, or if they meant GPLv2+

2. rpmlint says: 
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

So, the only issue here is the clarification of the license. 

I'll look at some of your other reviews in the mean time. ;)

Comment 9 François Cami 2010-08-14 19:13:17 UTC
Upstream pinged about the issue. I will update the bug, spec and srpm as needed.
Thanks a lot Kevin.

Comment 10 Kevin Fenzi 2010-08-15 19:59:54 UTC
Removing needsponsor here as I am sponsoring.

Comment 11 François Cami 2010-09-19 20:44:59 UTC
Kevin,

Would it be possible to decide to distribute the SRPM under the GPLv2, knowing that the throttle.spec file included in upstream's tarball mentions "License: GPL v2"? 

Just a thought.

Comment 12 Kevin Fenzi 2010-09-20 16:55:02 UTC
No word from upstream then I take it? :( 

I guess it's a reasonable bet that they mean GPLv2+, but it sure would be nice to at least confirm that in an email. ;(

Comment 13 François Cami 2010-09-20 17:02:33 UTC
upstream appears mute :/

I can maintain the code myself, so I don't have a problem with submitting and maintaining the package, but the license issue bites...
I would flag the license as GPLv2 since upstream's original spec mentions GPLv2, not v2+. What do you think?

Comment 14 Kevin Fenzi 2010-09-21 19:16:23 UTC
I'm leary of shipping something where we don't know what the author intends, but GPLv2 might be ok, since it's either GPLv2+ or GPLv2, and just GPLv2 would be the more restrictive one until we hear back. 

Adding FE-Legal here for Spot's opinion.

Comment 15 François Cami 2010-11-19 17:35:53 UTC
Upstream being unresponsive, I would like to ship under GPLv2 only.

Comment 16 Tom "spot" Callaway 2011-06-30 17:04:54 UTC
You can safely assume GPLv2 only. Lifting FE-Legal.

Comment 17 Kevin Fenzi 2011-06-30 17:15:02 UTC
With that cleared up, this package is APPROVED.

Comment 18 François Cami 2011-06-30 21:00:01 UTC
Thank you Tom and Kevin.

Comment 19 François Cami 2011-06-30 21:02:51 UTC
New Package SCM Request
=======================
Package Name: throttle
Short Description: Bandwidth limiting pipe
Owners: fcami 
Branches: f15 el5 el6
InitialCC:

Comment 20 Gwyn Ciesla 2011-07-01 12:12:24 UTC
Git done (by process-git-requests).

Comment 21 Fedora Update System 2011-07-01 21:45:05 UTC
throttle-1.2-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/throttle-1.2-4.fc15

Comment 22 Fedora Update System 2011-07-01 21:45:45 UTC
throttle-1.2-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/throttle-1.2-4.el6

Comment 23 Fedora Update System 2011-07-01 22:04:16 UTC
throttle-1.2-4.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/throttle-1.2-4.el5

Comment 24 Fedora Update System 2011-07-02 19:27:04 UTC
throttle-1.2-4.fc15 has been pushed to the Fedora 15 testing repository.

Comment 25 Fedora Update System 2011-07-12 05:24:08 UTC
throttle-1.2-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 26 Fedora Update System 2011-07-20 15:32:57 UTC
throttle-1.2-4.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 27 Fedora Update System 2011-07-20 15:34:10 UTC
throttle-1.2-4.el5 has been pushed to the Fedora EPEL 5 stable repository.