Bug 731219 (pipebench) - Review Request: pipebench - Measures the speed of stdin/stdout communication
Summary: Review Request: pipebench - Measures the speed of stdin/stdout communication
Keywords:
Status: CLOSED ERRATA
Alias: pipebench
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nathan Owe
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-08-17 02:27 UTC by Peter Gordon
Modified: 2011-09-30 18:32 UTC (History)
3 users (show)

Fixed In Version: pipebench-0.40-5.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-09-16 01:55:17 UTC
ndowens04: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Peter Gordon 2011-08-17 02:27:22 UTC
Spec URL: http://pgordon.fedorapeople.org/pipebench.spec
SRPM URL: http://pgordon.fedorapeople.org/pipebench-0.40-1.fc15.src.rpm

Description: Measures the speed of a pipe, by sitting in the middle passing the data along to the next process. See the included README for example usage.

If it helps, I also made a Koji scratch build, whose results can be seen at:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3278592

Thanks for your time.

Comment 1 Nathan Owe 2011-08-19 03:16:27 UTC
I have created a patch to use in place of the sed. As I have seen some reviewers prefer the patch over sed. Here is the patch if you would like to use it:

https://gitorious.org/ndowens-packager/master/blobs/master/patches/pipebench-0.40-Makefile-CFLAGS.patch

Comment 2 Nathan Owe 2011-08-19 03:39:18 UTC
Also in the files section why is it %{_mandir}/man?/ instead of %{_mandir}/man1/

Comment 3 Peter Gordon 2011-08-19 21:13:24 UTC
Thank you for your feedback, Nathan. With regards to using the sed invocation instead of the patch you provided, I appreciate the effort but I think it is unncessary. It's just a few search-and-replace changes; and I feel that using a patch instead would be adding another layer of complexity without just cause.

Also, I use the glob for the man pages as a convenience to myself, mostly force of habit, as I'm accustomed to larger packages where using the "man?/" pattern catches the man pages irrespective of the section they're installed into. It's a tiny bit of future-proofing that I think makes things a little nicer aesthetically. If you'd like me to explicitly list each section directory, I'd be happy to adjust that in the spec file. :)

Comment 4 Nathan Owe 2011-08-20 00:40:29 UTC
I ran a rpmlint on the x86_64 and it gave me 3 errors originally, first it said that stdin and stdout is misspelled, and in which we know isn't though to get around that error those two words could be capitalized like STDIN and STDOUT and the third error was that the COPYING file was incorrect-fsf-address. I looked that error up and it appears that the COPYING file provided has some issues, so to fix it like I read, I copied the text from the license from GNU's site and pasted it in a new COPYING file.  

I would suggest doing the above, especially on the COPYING file and put in comment that this fixes the issue of incorrect-fsf-address and have notified upstream.

Comment 5 Peter Gordon 2011-08-24 19:15:25 UTC
Thank you for the continued feedback, Nathan. I've fixed the STDIN/STDOUT spelling and added a patch to fix the GPLv2 text, which I've sent upstream (and noted in the spec).

Update spec and SRPM are available from my FedoraPeople webspace:
Spec URL: http://pgordon.fedorapeople.org/pipebench.spec
SRPM URL: http://pgordon.fedorapeople.org/pipebench-0.40-2.fc15.src.rpm

Comment 6 Nathan Owe 2011-08-25 00:45:01 UTC
What I would do instead of creating a patch for the COPYING file, I would simply include it as another Source, I have pasted the SPEC I have done:
http://pastebin.com/GBj74qZt

Comment 7 Peter Gordon 2011-08-25 19:57:44 UTC
(In reply to comment #6)
> What I would do instead of creating a patch for the COPYING file, I would
> simply include it as another Source, I have pasted the SPEC I have done:
> http://pastebin.com/GBj74qZt

Hmm..that's definitely simpler from the patch that I was using. I've updated the package per your suggestion.

Spec: http://pgordon.fedorapeople.org/pipebench.spec
SRPM: http://pgordon.fedorapeople.org/pipebench-0.40-2.fc15.src.rpm

Comment 8 Nathan Owe 2011-08-25 22:11:25 UTC
I am looking at the Review guidelines. One is about the License file, http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text, it sounds like upstream needs to provide the correct license for the package. Also would be better to list the source of the license like Source1: http://www.gnu.org/licenses/gpl-2.0.txt in my opinion.

Has upstream replied about the COPYING file yet?

Comment 9 Nathan Owe 2011-08-25 22:51:13 UTC
After emailing the devel mailing list, it seems that we can drop Source1 and ignore the incorrect-fsf-address error

Comment 10 Nathan Owe 2011-08-25 23:03:02 UTC
So going by the email, remove the Source1 line and remove lines:

## Update the included LICENSE file to match the current FSF GPLv2 text.
## (Fixes the FSF address and updates the "GNU Library GPL" references to "GNU
## Lesser GPL.") Submitted to upstream via email (2011-08-24).
install -D -m 0644 %{SOURCE1} LICENSE
and the %doc line should contain COPYING README

Comment 11 Peter Gordon 2011-08-26 04:46:07 UTC
Thanks for the check, Nathan. As Tom Callaway mentioned in his response dated August 5 (which Rahul linked to), "Incorrect FSF address is something that should be reported to upstreams. No further action is required of packagers, although, if a maintainer wishes to correct the address with a patch, they are certainly permitted to do so."

So I believe that the current fix should be OK. I'd prefer to keep the updated GPL text for clarity.

Thanks for your time in reviewing this.

Comment 12 Nathan Owe 2011-08-27 01:17:19 UTC
First off, the macros needs to be used consistently, so for example, if you are going to use %{name}, it needs to be used everywhere pipebench is used at, except of course Name:  pipebench :).

Also noticed that during mkdir for man dir: 
mkdir -m 755 -p %{buildroot}%{_mandir}/man1

I would specify that the man dir needs to have executive permission, so I would just do:
mkdir -p %{buildroot}%{_mandir}/man1

Also there is no need of the Group tag any more, though also doesn't say it must be removed, but in the wiki it says there is no use for it any more.

Also when I ran rpmlint on the SPEC file and SRPM, it gave a error with the Source0: and I found out it the Source listed needs to be http://www.habets.pp.se/synscan/files/%{name}-%{version}.tar.gz or w/o macro, whichever way chosen.

And of course changelog and revision needs to be +1 

Sorry, didn't notice these earlier.

Comment 13 Nathan Owe 2011-08-27 01:18:18 UTC
Also before I forget:

 pipebench.src:13: W: mixed-use-of-spaces-and-tabs (spaces: line 13, tab: line 1)

Comment 14 Peter Gordon 2011-09-03 16:31:27 UTC
Thanks for your patience, Nathan. I've fixed the issues you've mentioned and uploaded 0.40-4 to my FedoraPeople webspace with these changes:

Spec: http://pgordon.fedorapeople.org/pipebench.spec
SRPM: http://pgordon.fedorapeople.org/pipebench-0.40-4.fc15.src.rpm

Regards.

Comment 15 Nathan Owe 2011-09-06 01:34:15 UTC
[ndowens@revan rpmbuild]$ rpmlint ~/Downloads/pipebench-0.40-4.fc17.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[ndowens@revan rpmbuild]$ rpmlint SRPMS/pipebench-0.40-4.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Above is the rpmlint output

I have went through the review process and everything checks out, one macro left unchanged is http://www.habets.pp.se/synscan/programs.php?prog=pipebench
Change pipebench to %{name}

Other than that up the release and this package is then approved after.

Comment 16 Peter Gordon 2011-09-07 00:20:20 UTC
Oops - thanks for the catch. I've updated that as you suggested, and posted 0.40-5 to my webspace:

Spec: http://pgordon.fedorapeople.org/pipebench.spec
SRPM: http://pgordon.fedorapeople.org/pipebench-0.40-5.fc15.src.rpm

And thanks for the review/approval - please remember to set the 'fedora-review' flag as appropriate. :)

Comment 17 Nathan Owe 2011-09-07 15:37:12 UTC
You need to do the SCM request http://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 18 Peter Gordon 2011-09-07 16:07:27 UTC
Excellent - thanks for the time reviewing this, Nathan!

New Package SCM Request
=======================
Package Name: pipebench
Short Description: Measures the speed of stdin/stdout communication
Owners: pgordon
Branches: f14 f15 f16
InitialCC:

Comment 19 Gwyn Ciesla 2011-09-07 16:32:43 UTC
Git done (by process-git-requests).

Comment 20 Fedora Update System 2011-09-07 17:09:34 UTC
pipebench-0.40-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pipebench-0.40-5.fc16

Comment 21 Fedora Update System 2011-09-07 17:17:39 UTC
pipebench-0.40-5.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/pipebench-0.40-5.fc15

Comment 22 Fedora Update System 2011-09-07 17:29:05 UTC
pipebench-0.40-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/pipebench-0.40-5.fc14

Comment 23 Fedora Update System 2011-09-08 02:25:57 UTC
pipebench-0.40-5.fc16 has been pushed to the Fedora 16 testing repository.

Comment 24 Fedora Update System 2011-09-16 01:55:10 UTC
pipebench-0.40-5.fc15 has been pushed to the Fedora 15 stable repository.

Comment 25 Fedora Update System 2011-09-16 01:57:27 UTC
pipebench-0.40-5.fc14 has been pushed to the Fedora 14 stable repository.

Comment 26 Fedora Update System 2011-09-30 18:32:38 UTC
pipebench-0.40-5.fc16 has been pushed to the Fedora 16 stable repository.


Note You need to log in before you can comment on or make changes to this bug.