Spec URL: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm.spec SRPM URL: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm-1.00-2.fc6.src.rpm Description: SCSI disk parameters are held in mode pages. This utility lists or changes those parameters. Other SCSI devices (or devices that use the SCSI command set) such as CD/DVD and tape drives may also find parts of sdparm useful. Requires the linux kernel 2.4 series or later. In the 2.6 series any device node the understands a SCSI command set may be used (e.g. /dev/sda). In the 2.4 series SCSI device node may be used. Fetches Vital Product Data pages. Can send commands to start or stop the media and load or unload removable media.
As I recall, this is also useful for SATA devices. If it's alright with you, I think adding that to the %description and Summary ("List or change SCSI/SATA disk parameters") would be beneficial for those wanting to find this through a search or similar. Also, your %build section appears to be missing a make call: > make %{?_smp_mflags} Lastly, why is there a need for the checking of %buildroot in %clean/%install?
(In reply to comment #1) > As I recall, this is also useful for SATA devices. If it's alright with you, I > think adding that to the %description and Summary ("List or change SCSI/SATA > disk parameters") would be beneficial for those wanting to find this through a > search or similar. Fixed, better now? > Also, your %build section appears to be missing a make call: > > make %{?_smp_mflags} Indeed! > Lastly, why is there a need for the checking of %buildroot in %clean/%install? A upstream thingie, removed. New files: SPEC: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm.spec SRPMS: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm-1.00-3.fc6.src.rpm
Much better. Thanks. :)
I'll do a formal review of this tomorrow after classes.
Er. Sorry about the double comment there. How odd. :S
Okey dokey. Apologies for not getting to this sooner. Mock was being really weird last night. Let's get this party started (as the saying goes)... ---- ** MUST items ** GOOD: rpmlint is silent on the source and binary RPMs. GOOD: Package name and version follows the Naming Guidelines GOOD: The spec file matches the base package name: %{name}.spec GOOD: The package has an open-source compatible license (BSD) and meets the legal criteria for Fedora. The License tag in the spec file properly reflects this. GOOD: Spec file is written in American English and is legible (though I would align the tags at the top with spaces or tabs, but that's merely personal preference AFAIK, and definitely not a blocker in any way). GOOD: Source matches that of upstream. $ md5sum sdparm-1.00-*.tgz 1d46f85ed07e697f64fc40ddad31ddb5 sdparm-1.00-srpm.tgz 1d46f85ed07e697f64fc40ddad31ddb5 sdparm-1.00-upstream.tgz GOOD: Package successfully builds into binary RPMs on FC6/x86. GOOD: BuildRequires and Requires are correct.(The fact that they are not needed probably makes this a bit simpler. ^_^) GOOD: The %files section is okay. File and directory ownership does not conflict with system packages; and no duplicates are listed. The %defattr call is correct. GOOD: Package contains a %clean section, which consists of 'rm -rf %{buildroot}' GOOD: Macro usage is consistent. GOOD: Package contains code and permissible content. GOOD: %doc files do not affect runtime of program. ** SHOULD items ** GOOD: A copy of the license is included in the tarball as %doc ("COPYING"). GOOD: Package successfully builds in Mock for FC6 and Devel (both x86). GOOD: Packaged utility functions with no apparent errors or segfaults (tested with a WD Raptor SATA hard disk). ** Blockers ** BAD: The %changelog entries of those modifications before yours need to be made consistent with the Packaging Guidelines. See http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-b7d622f4bb245300199c6a33128acce5fb453213 for more information. BAD: The INSTALL file should not be packaged as %doc. Refer to http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b for more info. ** Not Applicable ** N/A: The package does not require ExcludeArch semantics. N/A: The package does not require %find_lang semantics, since it installs no locales. N/A: The package does not require %post/%postun calls to /sbin/ldconfig, since it installs no shared libraries. N/A: Package is not relocatable. N/A: There is no large documentation, so a -doc subpackage is not needed. N/A: No header files, shared or static library files, so no -devel subpackage is needed. Package installs no libtool archives. N/A: The package contains no pkgconfig (.pc) files. N/A: Not a GUI application, so no .desktop file needed. N/A: The package does not use translations, so no translated %description or Summary tag is available. N/A: No scriplets are used. N/A: No subpackages exist, so worries about fully-versioned Requires for those are not present. ---- I cannot sponsor you, but looking through other review requests you've posted for eterm and such, I see that Ed Hill sponsored you in bug #182175; so I am able to APPROVE this once you fix these two blockers (assuming that his sponsorship still stands).
> so I am > able to APPROVE this once you fix these two blockers (assuming that his > sponsorship still stands). Thanks, new files fixing %doc and changelog: SPEC: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm.spec SRPMS: http://www.pvv.ntnu.no/~terjeros/rpms/sdparm/sdparm-1.00-4.fc6.src.rpm
Good work. Those two issues are fixed and the rest is fine. This is therefore ACCEPTED. Please remember to close bug this as NEXTRELEASE once you've imported it into CVS and pushed it through the buildsystem.
"[...] to close bug this [...]" should be "to close this bug [...]" I need more caffeine apparently.
Package Change Request ====================== Package Name: sdparm New Branches: EL-4 EL-5 Owners: sharkcz
cvs done.