Bug 216519 - Review Request: sdparm - List or change SCSI disk parameters
Review Request: sdparm - List or change SCSI disk parameters
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Gordon
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-11-20 17:09 EST by Terje Røsten
Modified: 2009-10-03 17:33 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-24 14:24:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2006-11-20 17:09:50 EST
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.
Comment 1 Peter Gordon 2006-11-20 17:30:33 EST
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? 
Comment 2 Terje Røsten 2006-11-20 17:57:04 EST
(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
Comment 3 Peter Gordon 2006-11-20 18:08:26 EST
Much better. Thanks. :)
Comment 4 Peter Gordon 2006-11-22 01:04:30 EST
I'll do a formal review of this tomorrow after classes.
Comment 5 Peter Gordon 2006-11-22 01:05:13 EST
I'll do a formal review of this tomorrow after classes.
Comment 6 Peter Gordon 2006-11-22 01:08:13 EST
Er. Sorry about the double comment there. How odd. :S
Comment 7 Peter Gordon 2006-11-23 17:13:01 EST
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).
Comment 8 Terje Røsten 2006-11-23 17:28:21 EST
> 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

Comment 9 Peter Gordon 2006-11-23 18:02:04 EST
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.
Comment 10 Peter Gordon 2006-11-23 18:29:21 EST
"[...] to close bug this [...]" should be "to close this bug [...]"

I need more caffeine apparently.
Comment 11 Dan Horák 2009-10-01 13:05:08 EDT
Package Change Request
======================
Package Name: sdparm
New Branches: EL-4 EL-5
Owners: sharkcz
Comment 12 Kevin Fenzi 2009-10-03 17:33:27 EDT
cvs done.

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