Bug 520701 - Review Request: seeker - Random access disk benchmark utility
Summary: Review Request: seeker - Random access disk benchmark utility
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: David Timms
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-01 20:28 UTC by Ville Skyttä
Modified: 2009-10-08 18:04 UTC (History)
3 users (show)

Fixed In Version: 3.0-2
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-15 06:24:30 UTC
dtimms: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ville Skyttä 2009-09-01 20:28:22 UTC
http://scop.fedorapeople.org/packages/seeker.spec
http://scop.fedorapeople.org/packages/seeker-3.0-1.fc11.src.rpm

Seeker is a simple utility that reads small pieces of data from a raw
disk device in a random access pattern, and reports the average number
of seeks per second, and calculated random access time of the disk.

Comment 1 David Timms 2009-09-13 13:45:46 UTC
Since I have played with this tool, I was considering packaging it... you beat me to it. Review begins:
=====
OK  rpmlint seeker.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

OK  rpmlint seeker-3.0-1.fc11.src.rpm 
error checking signature of seeker-3.0-1.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

OK  name is non-conflicting with linux existing packages, fairly unique, first response in a google search.
    Note: a windows program exists with the name:
Seeker for Windows (Box+Download) (
An Immersive 3D Solar System Experience
Seeker is an immersive and interactive three dimensional solar simulator for your Windows Vista or XP computer. Experience simulated space flight and travel to, orbit and explore the Sun, Moon, planets, asteroids, and satellites with stunning realism...
  I don't think that is a problem since it isn't open source and there isn't linux version.

OK  spec named as package name.
OK  the original author asserts license: GNU General Public License v2. 
OK  spec specifies same license GPLv2.
OK  license file has been created using the author's quoting of intent, and included in %doc
OK  spec is legible english
OK  the upstream is a single c source file:
sha1sum seeker_baryluk.c ../upstream/seeker_baryluk.c 
5c472a283c499c053bc4610a63b2db6015b45263  seeker_baryluk.c
5c472a283c499c053bc4610a63b2db6015b45263  ../upstream/seeker_baryluk.c
OK  compiles to binaries on f11-i586, only arch I tested.
OK  rpmlint on built packages has no warnings nor errors.
OK   requires package glibc-headers, this is not in the requires exception list, but I think would be brought in automatically through other packages in the list. rpm -q --requires shows that libc (GLIBC) has been automatically added.

NA  no locales.
NA  no shared libs.
NA  not relocatable.
OK  owns files/folders that it creates, no duplicates.
OK  permissions: binary is executable and read only for normal users, doc is readonly for normal users. has the standard defattr line.
OK  clean standard line is included.
OK  is consistently using macros.
OK  code contains excecutable, allowed content.
OK  doc is very small
OK  %doc files are not part of the util.
OK  no header files, no static libraries, no pkconfig, no suffixed libs
OK  not a devel package, no .la archives.
OK  not a GUI app. 
OK  only owns files/folders it installs.
OK  install cleans buildroot
OK  filenames are good.
OK  utility runs and produces results as expected.
=====
So, pretty well right to go, with just a few things to clarify / queries:
?   Witold Baryluk's modification is the multithreaded version that is being packaged here. I think that explicitly stating that in the Description, and with the Source0: comment would be a good idea. It does seem to be the most appropriate version to package.

?   Would it be worth having a script that enumerates connected disks, and performs the test on each one, pausing at completion ? And providing a desktop file as an easy way to run it ?

?   why is the binary being placed in /usr/sbin ?

?   consider including in %doc the html saveas from the original web site, since it gives a decent background on usage. Most users wouldn't know to check the url/source to find out more. Not sure if that would be suitable for the package description (ie add URL).

Comment 2 Ville Skyttä 2009-09-13 21:10:30 UTC
(In reply to comment #1)
> ?   Witold Baryluk's modification is the multithreaded version that is being
> packaged here. I think that explicitly stating that in the Description, and
> with the Source0: comment would be a good idea.

Added, even though it might be subject to bitrot.

> ?   Would it be worth having a script that enumerates connected disks, and
> performs the test on each one, pausing at completion ? And providing a
> desktop file as an easy way to run it ?

Perhaps, patches welcome ;).  Not rocket science but the script should take root access requirement into account.  But I think this can wait until post-review.  Since you were considering packaging it, I can happily hand it over for you to maintain and improve, or co-maintain if you prefer that...

> ?   why is the binary being placed in /usr/sbin ?

I assume you mean compared to placing it in /usr/bin?  Because running it requires root access and I don't want to clutter normal users' PATH with it in setups that don't have /usr/sbin there.  With recent Fedora defaults, /usr/sbin vs /usr/bin doesn't make much practical difference at all because both are in default PATH and I think this tool fits better in the former anyway.

> ?   consider including in %doc the html saveas from the original web site,
> since it gives a decent background on usage.

Done as upstream gives CC BY-SA 2.5 permission to do that at http://www.linuxinsight.com/about.html

New srpm at http://scop.fedorapeople.org/packages/seeker-3.0-2.fc11.src.rpm, specfile URL unchanged.

* Sun Sep 13 2009 Ville Skyttä <ville.skytta@iki.fi> - 3.0-2
- Address review comments (#520701):
- Improve %description.
- Include upstream article in docs.

Comment 3 David Timms 2009-09-14 13:58:41 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > ?   Witold Baryluk's modification is the multithreaded version that is being
> > packaged here. I think that explicitly stating that in the Description, and
> > with the Source0: comment would be a good idea.
> 
> Added, even though it might be subject to bitrot.
OK.

> > ?   Would it be worth having a script that enumerates connected disks, and
> > performs the test on each one, pausing at completion ? And providing a
> > desktop file as an easy way to run it ?
> 
> Perhaps, patches welcome ;).  Not rocket science but the script should take
> root access requirement into account.  But I think this can wait until
> post-review.  Since you were considering packaging it, I can happily hand it
> over for you to maintain and improve, or co-maintain if you prefer that...
I only thought of this because I did something similar for another one I just submitted for review (tnef #522920). bash/gnome-desktop isn't my fort'e, so any comments on that would be welcome. Will keep it in mind for later.

> > ?   consider including in %doc the html saveas from the original web site,
> > since it gives a decent background on usage.
> 
> Done as upstream gives CC BY-SA 2.5 permission to do that at
> http://www.linuxinsight.com/about.html
Wow, that looks great (like better than the original web site), I'll have to remember tidy as a useful tool. OK.

> New srpm at http://scop.fedorapeople.org/packages/seeker-3.0-2.fc11.src.rpm,
> specfile URL unchanged.
Checked updated srpm. rpmlints OK, compiles, runs OK. All queries answered, and package updated as suggested.
=====
As I can see Ville is an existing contributor and sponsor in Fedora,
I (David Timms) hereby approve the package seeker.
=====

Comment 4 Ville Skyttä 2009-09-14 20:19:32 UTC
Thanks for the review.  As said, feel free to apply for co-maintainership if you wish in pkgdb, or let me know if you'd like primary maintainership.

New Package CVS Request
=======================
Package Name: seeker
Short Description: Random access disk benchmark utility
Owners: scop
Branches: F-11 EL-5
InitialCC:

Comment 5 Kevin Fenzi 2009-09-14 21:36:29 UTC
cvs done.

Comment 6 Ville Skyttä 2009-09-15 06:24:30 UTC
Rawhide build done, F-11 and EL-5 will follow later.  Thanks.
http://koji.fedoraproject.org/koji/taskinfo?taskID=1679556

Comment 7 Fedora Update System 2009-09-15 17:29:01 UTC
seeker-3.0-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/seeker-3.0-2.fc11

Comment 8 Fedora Update System 2009-09-15 17:30:09 UTC
seeker-3.0-2.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/seeker-3.0-2.el5

Comment 9 Fedora Update System 2009-10-03 19:12:09 UTC
seeker-3.0-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 10 Fedora Update System 2009-10-08 18:04:45 UTC
seeker-3.0-2.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


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