Bug 225882 - Merge Review: hdparm
Merge Review: hdparm
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Karsten Hopp
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-01-31 14:03 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

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

Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:03:04 EST
Fedora Merge Review: hdparm

Initial Owner: karsten@redhat.com
Comment 1 Andy Grimm 2007-02-03 16:43:50 EST
I'm reviewing this one (not sponsored yet)
Comment 2 Andy Grimm 2007-02-03 17:34:42 EST
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


* LICENSE.TXT must be included as a %doc

Other things look fine.

* 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 11:22:51 EST
should be fixed in hdparm-6.9-2, thanks for the review !
I'll leave this report open as suggested in 
Comment 4 Kevin Fenzi 2007-02-08 22:57:07 EST
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 09:18:23 EST
- 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 09:55:54 EST
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 10:11:31 EST
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 16:46:22 EST
- 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
- 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

- 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.


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