Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225882 - Merge Review: hdparm
Summary: Merge Review: hdparm
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Karsten Hopp
QA Contact: Fedora Package Reviews List
Depends On:
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:
Clone Of:
Last Closed: 2007-06-20 07:54:53 UTC
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
Initial Owner:

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


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

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