Bug 520701
Summary: | Review Request: seeker - Random access disk benchmark utility | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ville Skyttä <ville.skytta> |
Component: | Package Review | Assignee: | David Timms <dtimms> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dtimms, fedora-package-review, notting |
Target Milestone: | --- | Flags: | dtimms:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 3.0-2 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-09-15 06:24:30 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
Ville Skyttä
2009-09-01 20:28:22 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). (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> - 3.0-2 - Address review comments (#520701): - Improve %description. - Include upstream article in docs. (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. ===== 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: cvs done. Rawhide build done, F-11 and EL-5 will follow later. Thanks. http://koji.fedoraproject.org/koji/taskinfo?taskID=1679556 seeker-3.0-2.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/seeker-3.0-2.fc11 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 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. 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. |