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 Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
First review: https://bugzilla.redhat.com/show_bug.cgi?id=575529 Updated Spec: http://fcami.fedorapeople.org/srpms/throttle.spec Updated SRPM: http://fcami.fedorapeople.org/srpms/throttle-1.2-3.fc13.src.rpm 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. 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) 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. Another package review request: https://bugzilla.redhat.com/show_bug.cgi?id=624179 Another package review request: https://bugzilla.redhat.com/show_bug.cgi?id=624182 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. ;) Upstream pinged about the issue. I will update the bug, spec and srpm as needed. Thanks a lot Kevin. Removing needsponsor here as I am sponsoring. 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. 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. ;( 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? 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. Upstream being unresponsive, I would like to ship under GPLv2 only. You can safely assume GPLv2 only. Lifting FE-Legal. With that cleared up, this package is APPROVED. Thank you Tom and Kevin. New Package SCM Request ======================= Package Name: throttle Short Description: Bandwidth limiting pipe Owners: fcami Branches: f15 el5 el6 InitialCC: Git done (by process-git-requests). throttle-1.2-4.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/throttle-1.2-4.fc15 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 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 throttle-1.2-4.fc15 has been pushed to the Fedora 15 testing repository. throttle-1.2-4.fc15 has been pushed to the Fedora 15 stable repository. throttle-1.2-4.el6 has been pushed to the Fedora EPEL 6 stable repository. throttle-1.2-4.el5 has been pushed to the Fedora EPEL 5 stable repository. |