Bug 225882 - Merge Review: hdparm
Summary: Merge Review: hdparm
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Karsten Hopp
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 19:03 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-20 07:54:53 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 19:03:04 UTC
Fedora Merge Review: hdparm

http://cvs.fedora.redhat.com/viewcvs/devel/hdparm/
Initial Owner: karsten@redhat.com

Comment 1 Andy Grimm 2007-02-03 21:43:50 UTC
I'm reviewing this one (not sponsored yet)

Comment 2 Andy Grimm 2007-02-03 22:34:42 UTC
This one has several rpmlint errors:

rpmlint on ./hdparm-6.9-1.src.rpm
W: hdparm summary-ended-with-dot A utility for displaying and/or setting hard
disk parameters.
   - Remove the dot

E: hdparm tag-not-utf8 %changelog
E: hdparm non-utf8-spec-file hdparm.spec
   - make sure that umlauted characters and such are utf8-encoded, not latin-1

E: hdparm no-cleaning-of-buildroot %install
   - clean $RPM_BUILD_ROOT at the beginning of %install.

W: hdparm mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 9)
W: hdparm patch-not-applied Patch2: hdparm-6.3-idestruct.patch
  - Remove the patch if it's no longer needed

Also:

* LICENSE.TXT must be included as a %doc

Other things look fine.

Checked:
* md5sum matches upstream
* specfile is clean
* license is acceptable
* buildrequires is fine
* no locales, no shlibs, no relocations, etc.  

Comment 3 Karsten Hopp 2007-02-05 16:22:51 UTC
should be fixed in hdparm-6.9-2, thanks for the review !
I'll leave this report open as suggested in 
http://fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags?highlight=%28Review%29

Comment 4 Kevin Fenzi 2007-02-09 03:57:07 UTC
In reply to comment #3:

Yeah, since that was a review from a not yes sponsored reviewer, it should be 
left open for an official review. 

Thanks for looking at this Andy!

Comment 5 manuel wolfshant 2007-02-09 14:18:23 UTC
- BuildRoot is not the preferred value
- %build does not use smp flags; if not supported, please add a comment in the spec
- please also consider including README.accoustic; the feature is documented in
the man page, but the README seems to include a bit more info

The rest seems OK. Please fix the above and I'll do a full review.


Comment 6 Karsten Hopp 2007-02-09 14:55:54 UTC
ok, fixed in hdparm-6_9-3. I've added smp flags although it doesn't really
matter with just 2 .c files and a built time of <3 secs

Comment 7 manuel wolfshant 2007-02-09 15:11:31 UTC
I know and I agree with you. It's a matter of consistency among packages... and
maybe even insurance for the future.

I'll review after the servers sync.

Comment 8 manuel wolfshant 2007-02-10 21:46:22 UTC
MUST:
- package meets naming guidelines
- package meets packaging guidelines
- license (BSD) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all files/directories that it creates, does not take ownership of foreign
files/dirs
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 
- just a small binary, no static, .la, .pc etc
- no scriptlets

SHOULD
- builds in mock
- runs as advertised

The only caveat I see is that, despite being passed RPM_BUILD_OPT in %build, it
also takes looooots of compile parameters from the Makefile. All seem sane
however, and are not very important since it's not an application from which to
squeeze each ounce of performance.

APPROVED




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