[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=700571

Hans de Goede <hdegoede at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review?

--- Comment #3 from Hans de Goede <hdegoede at redhat.com> 2011-04-29 08:46:50 EDT ---
Full review done:

Good:
- rpmlint checks return:
spindown.src: W: name-repeated-in-summary C spindown
spindown.src: W: spelling-error %description -l en_US sg -> chg, gs, sf
spindown.src: W: spelling-error %description -l en_US swappable -> trappable
spindown.src: W: spelling-error %description -l en_US hda -> had, ha, Ida
spindown.src: W: spelling-error %description -l en_US sdb -> db, sob, sub
spindown.src: W: invalid-url Source0:
http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not
Found
spindown.x86_64: W: name-repeated-in-summary C spindown
spindown.x86_64: W: spelling-error %description -l en_US sg -> chg, gs, sf
spindown.x86_64: W: spelling-error %description -l en_US swappable -> trappable
spindown.x86_64: W: spelling-error %description -l en_US hda -> had, ha, Ida
spindown.x86_64: W: spelling-error %description -l en_US sdb -> db, sob, sub
spindown.x86_64: W: no-documentation
spindown.x86_64: W: no-manual-page-for-binary spindownd
 These can all be ignored
- package meets naming guidelines
- package meets packaging guidelines
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- no need for .desktop file

Needs work:
- rpmlint checks return:
spindown-debuginfo.x86_64: E: debuginfo-without-sources
 That is not good, this is likely caused by RPM_OPT_FLAGS not being used, see
 below.

-license should be GPLv3+

-the requires for the post / preun scripts are wrong, and the output
 of the service command should be silenced, please use the template requires
 + scripts from here:

http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

-RPM_OPT_FLAGS are not used during compilation. You call make with:
 OPT="$RPM_OPT_FLAGS" but that does not seem to do the trick, the output during
 building shows:
 g++ -O1 -c src/main.cpp

-You're not using %{?_smp_mflags} when calling make from %build, if you've
tried
 so and it does not work, please add a comment stating so

-src/ininiparser3.0b contains an embedded copy if the iniparser library,
 luckily the exact same version is already packaged for Fedora, so fixing this
 is easy, just rm -rf src/ininiparser3.0b in %prep, fixes the includes to use
 the headers from the iniparser-devel package and add -liniparser to the
 linking command in the Makefile

-the initscript:
 - tries to start shutdownd when no /etc/shutdown.conf is present
 - is missing the usual printing of [ OK] / [FAILED]
 - you may want to consider writing a Fedora specific initscript, using
   the template from:
   http://fedoraproject.org/wiki/Packaging/SysVInitScript
   That should be easy to adapt to shutdownd

- You're not packaing any docs, you MUST package the COPYING file as %doc,
  you should also package the CHANGELOG README TODO and spindown.conf.example
  files as %doc

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.