[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Fedora Update System changed: What|Removed |Added Fixed In Version|spindown-0.4.0-3.fc14 |spindown-0.4.0-3.fc15 -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #14 from Fedora Update System 2011-06-08 20:06:28 EDT --- spindown-0.4.0-3.fc15 has been pushed to the Fedora 15 stable repository. -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Fedora Update System changed: What|Removed |Added Status|ON_QA |CLOSED Fixed In Version||spindown-0.4.0-3.fc14 Resolution||ERRATA Last Closed||2011-05-24 22:49:56 -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #13 from Fedora Update System 2011-05-24 22:49:51 EDT --- spindown-0.4.0-3.fc14 has been pushed to the Fedora 14 stable repository. -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Fedora Update System changed: What|Removed |Added Status|MODIFIED|ON_QA --- Comment #12 from Fedora Update System 2011-05-13 19:16:17 EDT --- spindown-0.4.0-3.fc14 has been pushed to the Fedora 14 testing repository. -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #11 from Fedora Update System 2011-05-13 05:31:29 EDT --- spindown-0.4.0-3.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/spindown-0.4.0-3.fc14 -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #10 from Fedora Update System 2011-05-13 05:27:23 EDT --- spindown-0.4.0-3.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/spindown-0.4.0-3.fc15 -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Fedora Update System changed: What|Removed |Added Status|ASSIGNED|MODIFIED -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #9 from Jason Tibbitts 2011-05-10 11:40:45 EDT --- Git done (by process-git-requests). -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Martin Cermak changed: What|Removed |Added Flag||fedora-cvs? --- Comment #8 from Martin Cermak 2011-05-07 06:43:36 EDT --- New Package SCM Request === Package Name: spindown Short Description: Daemon that can spin idle disks down Owners: mcermak Branches: f14 f15 InitialCC: jwrdegoede -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 changed: What|Removed |Added Flag|fedora-review? |fedora-review+ --- Comment #7 from Hans de Goede 2011-05-06 09:17:50 EDT --- (In reply to comment #6) > Created spindown-0.4.0-3.fc14 addressing the aforementioned issues, see [1]. > I hope it'll be fine now. Please review. > > --- > [1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-3/ Hi, Thanks, looks good now: approved. Regards, Hans -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #6 from Martin Cermak 2011-05-06 08:27:30 EDT --- Created spindown-0.4.0-3.fc14 addressing the aforementioned issues, see [1]. I hope it'll be fine now. Please review. --- [1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-3/ -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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
--- Comment #5 from Hans de Goede 2011-05-06 05:22:13 EDT
---
Much better now, almost there.
Needs work:
- You're installing the example config file as /etc/spindown.conf now,
no need to include at is as %doc then
- Thanks for doing the new initscript, but I'm afraid it needs some work:
Must Fix:
- The LSB HEADER is entirely empty, fill in the Short Description and
Description and remove all the other lines.
- statuspipe="/tmp/spindown.status"
Is vulnerable to symlinks attacks, please make this
/var/run/spindown.status
instead
- the $prog in "status -p /var/run/spindownd.pid $prog" must be $exec
- start() is missing a "return $retval" at the end
Should fix:
- Given that you've effectively written a new script, please add it as
Source1: (without an url) and then in $setup do:
cp -p %{SOURCE1} .
Rather then making it a hard to read patch. This will also make
making future changes to it a lot easier.
- Drop the unused RETVAL variable
- Please use a pidfile variable (like lockfile and statuspipe) rather then
writing out /var/run/spindownd.pid in full everywhere
- This part:
echo -n $"Starting $prog: "
if [ $UID -ne 0 ] ; then
failure
echo -e "\nUser has insufficient privilege."
exit 4
fi
if [ ! -x $exec ]; then
failure
echo -e "\nDaemon binary not executable."
exit 5
fi
if [ ! -f $config ]; then
failure
echo -e "\nConfig file missing."
exit 6
fi
Deviates from how most Fedora init scripts do it, please change this to:
[ $UID -eq 0 ] || exit 4
[ -x $exec ] || exit 5
[ -f $config ] || exit 6
echo -n $"Starting $prog: "
- You can likely (and if it works should) write this bit:
$exec -d -f $statuspipe -c $config -p /var/run/spindownd.pid;
retval=$?
[ "$retval" -eq 0 ] && success || failure
echo
As:
daemon $exec -d -f $statuspipe -c $config -p /var/run/spindownd.pid;
retval=$?
echo
- stop() can be simplified to just:
stop() {
echo -n $"Stopping $prog: "
killproc -p /var/run/spindownd.pid $exec
retval=$?
echo
[ $retval -eq 0 ] && rm -f $lockfile
return $retval
}
- judging from the old initscript reload should be:
reload() {
killproc -p /var/run/spindownd.pid $exec -HUP
}
And force_reload should then call reload rather then restart
--
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.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #4 from Martin Cermak 2011-05-05 10:02:55 EDT --- I belive all issues are fixed now in spindown-0.4.0-2: Spec URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown.spec SRPM URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-2.fc14.src.rpm Patch0 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-initscript.patch Patch1 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-Makefile.patch Patch2 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-iniparser.patch All changes are described in the changelog. Because the project is not under version control yet, I'm creating patches against pure sources. For this reason patches changed (for example [1] differs from [2]). Also I'm omitting the licence change announcement on fedora-devel-list in this case. Here are rpmlint results: + rpmlint spindown.spec spindown.spec: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found 0 packages and 1 specfiles checked; 0 errors, 1 warnings. + rpmlint spindown-0.4.0-2.fc14.src.rpm spindown.src: W: spelling-error %description -l en_US sg -> SG, chg, cig spindown.src: W: spelling-error %description -l en_US hdparm -> hardpan, harmed, Parma spindown.src: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable spindown.src: W: spelling-error %description -l en_US hda -> had, hd, ha spindown.src: W: spelling-error %description -l en_US sdb -> sd, sb, db spindown.src: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found 1 packages and 0 specfiles checked; 0 errors, 6 warnings. + rpmlint spindown-debuginfo-0.4.0-2.fc14.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. + rpmlint spindown-0.4.0-2.fc14.x86_64.rpm spindown.x86_64: W: spelling-error %description -l en_US sg -> SG, chg, cig spindown.x86_64: W: spelling-error %description -l en_US hdparm -> hardpan, harmed, Parma spindown.x86_64: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable spindown.x86_64: W: spelling-error %description -l en_US hda -> had, hd, ha spindown.x86_64: W: spelling-error %description -l en_US sdb -> sd, sb, db spindown.x86_64: W: no-manual-page-for-binary spindownd 1 packages and 0 specfiles checked; 0 errors, 6 warnings. The package does not explicitly require sg3_utils because custom power management binaries (like sg_start or hdparm) can be used. I also performed sanity testing of the package on a machine with multiple disks. --- [1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-Makefile.patch [2] http://www.physics.muni.cz/~cermak/spindown/spindown-0.4.0-Makefile.patch -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 changed:
What|Removed |Added
Flag||fedora-review?
--- Comment #3 from Hans de Goede 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.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 changed: What|Removed |Added Status|NEW |ASSIGNED CC||hdego...@redhat.com Blocks|177841(FE-NEEDSPONSOR) | AssignedTo|nob...@fedoraproject.org|hdego...@redhat.com --- Comment #2 from Hans de Goede 2011-04-29 08:21:41 EDT --- I'll review this (and your other submission), and assuming all goes well sponsor you eventually, removing FE_NEEDSPONSOR blocker. -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 --- Comment #1 from Martin Cermak 2011-04-29 03:19:55 EDT --- > spindown.spec: W: invalid-url Source0: > http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: > Not Found Just note this is a valid url, but google blocks certain clients probably. Also the spelling warnings should be okay I suppose. -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 700571] Review Request: spindown - Daemon that can spindown idle disks
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 Martin Cermak changed: What|Removed |Added Blocks||177841(FE-NEEDSPONSOR) -- 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. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
