[Bug 197967] New: Review Request: gkrellm - Multiple stacked system monitors in one process
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: bugzilla-sink@leemhuis.info ReportedBy: j.w.r.degoede@hhs.nl QAContact: fedora-package-review@redhat.com
Spec URL: http://people.atrpms.net/~hdegoede/gkrellm.spec SRPM URL: http://people.atrpms.net/~hdegoede/ Description: GKrellM charts SMP CPU, load, Disk, and all active net interfaces automatically. An on/off button and online timer for the PPP interface is provided. Monitors for memory and swap usage, file system, internet connections, APM laptop battery, mbox style mailboxes, and cpu temps. Also includes an uptime monitor, a hostname label, and a clock/calendar. Additional features are:
* Autoscaling grid lines with configurable grid line resolution. * LED indicators for the net interfaces. * A gui popup for configuration of chart sizes and resolutions.
Note 1: This packages is moving from core to extras! The core packages also includes the gkrellm wireless plugin, but that _really_ should be packaged seperatly as its a completly seperate tarbal. Expect a review request for gkrellm-wireless soon (ish)
Note 2: I'm going on a short vacation from monday 10 juli till friday 14 juli, so if I'm quiet thats why.
Note 3, rpmlint output: W: gkrellm-daemon dangerous-command-in-%postun userdel Needed, can be ignored W: gkrellm-daemon incoherent-init-script-name gkrellmd The initscript is / was called gkrellmd in FC, changing the name to be consistent with the package name would cause more pain then its worth. W: gkrellm-devel no-documentation There are no development related docs to package
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From ville.skytta@iki.fi 2006-07-10 15:57 EST ------- I'm a firm believer in that users/groups created by packages should *not* be erased when the package is removed, and won't set myself as the reviewer because of that. Let me know if you're willing to leave them behind when erasing -daemon, and I'll take care of the review.
Anyway, a couple of comments:
The switch to lm_sensors means that ppc will need special treatment, as lm_sensors is not (nor obviously -devel) available for it. ExcludeArch: ppc would sound like a regression.
-daemon has grown a "Provides: gkrellm-server" for no apparent reason, and -daemon has become self-obsoleting because of that, which may or may not cause problems. I'd suggest removing it and adding a "< $some_known_version-$release" to Obsoletes: gkrellm-server.
There's a mismatch between %install and %files where the former uses various hardcoded paths while the latter uses macros (at least /etc vs %{_sysconfdir}).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-14 14:57 EST ------- (In reply to comment #1)
I'm a firm believer in that users/groups created by packages should *not* be erased when the package is removed, and won't set myself as the reviewer because of that. Let me know if you're willing to leave them behind when erasing -daemon, and I'll take care of the review.
Sure, that sounds reasonable, how does one pick up the leftover user when the user decides to reinstall the package later?
Anyway, a couple of comments:
The switch to lm_sensors means that ppc will need special treatment, as lm_sensors is not (nor obviously -devel) available for it. ExcludeArch: ppc would sound like a regression.
That patch was written in communicatiopn with upstream and is going to appear in the next upstream release. All the changes are wrapped in #ifdef HAVE_LIBSENSORS, hence the adding of -DHAVE_LIBSENSORS to CFLAGS. I'll make the adding off that conditionally with %ifarch, does that sound ok?
-daemon has grown a "Provides: gkrellm-server" for no apparent reason, and -daemon has become self-obsoleting because of that, which may or may not cause problems. I'd suggest removing it and adding a "< $some_known_version-$release" to Obsoletes: gkrellm-server.
Erm rpmlint complains without it. I'll remove the Provides. Are you sure this makes a package self obsoleting?
There's a mismatch between %install and %files where the former uses various hardcoded paths while the latter uses macros (at least /etc vs %{_sysconfdir}).
I'll do a new version with %{_sysconfdir} everywhere + other fixes once I've got a reply to my above remarks / questions.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From ville.skytta@iki.fi 2006-07-14 15:29 EST ------- (In reply to comment #2)
Sure, that sounds reasonable, how does one pick up the leftover user when the user decides to reinstall the package later?
It already works that way due to "|| :" in the useradd/groupadd lines.
All the changes are wrapped in #ifdef HAVE_LIBSENSORS, hence the adding of -DHAVE_LIBSENSORS to CFLAGS. I'll make the adding off that conditionally with %ifarch, does that sound ok?
Fine with me; I assume the sensors functionality wasn't available on PPC in earlier versions either. Someone who uses this on PPC, please correct if I'm wrong. Oh, and BR: lm_sensors-devel needs to be %ifarch'ed too.
Erm rpmlint complains without it. I'll remove the Provides.
Actually, I think it's time for both Provides/Obsoletes: gkrellm-server to go. The obsoletes was added in a patch I submitted almost three years ago (see Oct 09 2003 in %changelog), it was added back then in order to provide clean upgrades from some 3rd party packages, but I no longer remember the details.
Are you sure this makes a package self obsoleting?
It does obsolete something that it also provides, and I cannot come up with a reason why doing so would ever be desirable. Modern depsolvers probably won't choke on/ do odd things with that any more though.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
cweyl@alumni.drew.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |197981 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-15 02:12 EST ------- Created an attachment (id=132475) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=132475&action=vie...) New specfile fixing discussed issues
Here is a new specfile, sorry no URL, because I don't have upload access from this PC.
Changes: * Sat Jul 15 2006 Hans de Goede j.w.r.degoede@hhs.nl 2.2.9-5 - Remove Obsoletes/Provides gkrellm-server - Don't remove user on uninstall - Only build with lm_sensors support on x86 / x86_64 since lm_sensors is not available on other archs. - Use %%{_sysconfdir} instead of /etc in %%install
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|bugzilla-sink@leemhuis.info |ville.skytta@iki.fi OtherBugsDependingO|163776 |163778 nThis| |
------- Additional Comments From ville.skytta@iki.fi 2006-07-15 05:07 EST ------- - "standard" BuildRoot not used - useradd/groupadd dependencies missing - chkconfig dependencies should be context marked, and chkconfig called consistently (with full path) - using a macro for %{flags} seems a bit odd, normal shell variables should work just fine - %{?_smp_mflags} missing - could use %{_initrddir} for the init script location - could use init script directly in %preun daemon - "SMP CPU" sounds odd in the description (gkrellm does UP CPU too), and it could be improved a bit otherwise too
Will attach a patch for the above.
Another random note (for upstream?):
- gkrellm.pc doesn't look very useful at the moment. For plugin/theme packages it would be nice to have the lib and data dirs defined in it, for example by adding pkgdatadir=%{_datadir}/gkrellm2 and pkglibdir=%{_libdir}/gkrellm2 in it; then those could be queried like pkg-config gkrellm --variable=pkglibdir
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From ville.skytta@iki.fi 2006-07-15 05:09 EST ------- Created an attachment (id=132478) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=132478&action=vie...) Suggested patch for findings in comment 5
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From ville.skytta@iki.fi 2006-07-15 05:13 EST ------- Oh, and gkrellmd should be condrestarted on -daemon upgrades. I'd also add LSB action aliases to the init script (try-restart -> condrestart, force-reload -> restart).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-15 07:01 EST ------- (In reply to comment #5) Thanks for the patch, I've applied it, but I've undone this:
- could use init script directly in %preun daemon
I agree, but for cases where a full example is given on the ScriptletSnippets wiki page, I always use the code from the wiki in the name of consistency across FE as a whole. And the ScriptletSnippets wiki page uses /sbin/service: http://fedoraproject.org/wiki/ScriptletSnippets
Also the current code in the wiki doesn't use " || :", so neither does this version of gkrellm (for the service stuff), that can be fixed if you want though.
Another random note (for upstream?):
- gkrellm.pc doesn't look very useful at the moment. For plugin/theme packages it would be nice to have the lib and data dirs defined in it, for example by adding pkgdatadir=%{_datadir}/gkrellm2 and pkglibdir=%{_libdir}/gkrellm2 in it; then those could be queried like pkg-config gkrellm --variable=pkglibdir
I'll send this upstream.
(In reply to comment #7)
Oh, and gkrellmd should be condrestarted on -daemon upgrades. I'd also add LSB action aliases to the init script (try-restart -> condrestart, force-reload -> restart).
Done and done.
New version at: Spec URL: http://people.atrpms.net/~hdegoede/gkrellm.spec SRPM URL: http://people.atrpms.net/~hdegoede/gkrellm-2.2.9-6.src.rpm
Changes: * Sat Jul 15 2006 Hans de Goede j.w.r.degoede@hhs.nl 2.2.9-6 - Various specfile improvements by Ville Skyttä (ville.skytta@iki.fi) - Make the daemon package scripts match the ScriptletSnippets wiki page - Add LSB aliases (try-restart, force-reload) to the -daemon initscript - Add %%{?dist} to the release for consistency with other packages I maintain
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From ville.skytta@iki.fi 2006-07-16 10:55 EST ------- Yes, "|| :" should be added to condrestart. And IMO the snippets page should be fixed to use the scripts directly, I see no benefit from using /sbin/service instead of them. Some people have mentioned that *not* using it makes it easier to get rid of initscripts altogether and to use another init system. I don't know the details and that wouldn't work without other changes anyway, so it's not a blocker.
Other issues:
- Seems that there's no need to require the main package in -devel, nothing in it depends on anything in main, right?
- Desktop file has been renamed from gnome-gkrellm.desktop to fedora-gkrellm.desktop, which will break eg. buttons added to the KDE panel from menus using the "add application to panel" function, and I believe there are other similar problems in other desktops if that's done, so I suggest reverting the rename.
- Regression in desktop entry: StartupNotify=false prevents KDE's built-in startup notification from working. The key should be just removed.
- The default gkrellmd.conf uses "proc" as the group to drop privs to. That group doesn't exist, should probably be gkrellmd instead.
- groupadd should be done with the -r argument.
- The switch from the builtin sensors stuff to libsensors appears to break existing sensors configs, my configured sensors just disappeared from gkrellm (but reconfiguring the sensors worked). Would there be a sane way to prevent this? If not, not a blocker.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-16 14:41 EST ------- (In reply to comment #9)
Yes, "|| :" should be added to condrestart.
I agree, I've added all gkrellmd related scripts to use || : as is usual for scriptlets. Maybe we should update the wiki for this?
And IMO the snippets page should be fixed to use the scripts directly, I see no benefit from using /sbin/service instead of them. Some people have mentioned that *not* using it makes it easier to get rid of initscripts altogether and to use another init system. I don't know the details and that wouldn't work without other changes anyway, so it's not a blocker.
I have no opinion on this, I guess this should be discussed on f-e-l.
Other issues:
- Seems that there's no need to require the main package in -devel, nothing in it depends on anything in main, right?
Agreed, fixed.
- Desktop file has been renamed from gnome-gkrellm.desktop to fedora-gkrellm.desktop, which will break eg. buttons added to the KDE panel from menus using the "add application to panel" function, and I believe there are other similar problems in other desktops if that's done, so I suggest reverting the rename.
Agreed, fixed.
- Regression in desktop entry: StartupNotify=false prevents KDE's built-in startup notification from working. The key should be just removed.
Done.
- The default gkrellmd.conf uses "proc" as the group to drop privs to. That group doesn't exist, should probably be gkrellmd instead.
Fixed.
- groupadd should be done with the -r argument.
Fixed.
- The switch from the builtin sensors stuff to libsensors appears to break existing sensors configs, my configured sensors just disappeared from gkrellm (but reconfiguring the sensors worked). Would there be a sane way to prevent this? If not, not a blocker.
I'm sorry but that would be very non-trivial to fix, it would probably require some kinda fuzzy logic and never work reliable in all cases -> EWONTFIX
Here is a new version: Spec URL: http://people.atrpms.net/~hdegoede/gkrellm.spec SRPM URL: http://people.atrpms.net/~hdegoede/gkrellm-2.2.9-7.src.rpm
Changes: * Sun Jul 16 2006 Hans de Goede j.w.r.degoede@hhs.nl 2.2.9-7 - Add -r to groupadd - Add || : to the gkrellmd service related scripts (deviation from the wiki). - Don't make -devel package require the main one as it doesn't need it - Install .desktop file with --vendor gnome to not break existing kde panel buttons, etc. - Drop "StartupNotify=false" from .desktop to not interfere with kde's internal startup notification - use gkrellmd as group in default gkrellmd.conf
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |karsten@redhat.com
------- Additional Comments From ville.skytta@iki.fi 2006-07-16 15:58 EST ------- I've added some '|| :'s to the service commands in the scriptlet snippets page, those are usual suspects for failing.
2.2.9-7 looks good, approved, also per discussion in the thread starting at http://www.redhat.com/archives/fedora-devel-list/2006-July/msg00012.html
Karsten, FYI: gkrellm is ready to be imported to Extras, so it can be removed from Core devel soon.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163778 |163779 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-16 16:01 EST ------- (In reply to comment #11)
I've added some '|| :'s to the service commands in the scriptlet snippets page, those are usual suspects for failing.
Good!
2.2.9-7 looks good, approved, also per discussion in the thread starting at http://www.redhat.com/archives/fedora-devel-list/2006-July/msg00012.html
Thanks, I'll import it and request a build.
Karsten, FYI: gkrellm is ready to be imported to Extras, so it can be removed from Core devel soon.
Erm, hold on first gkrellm-wifi which is hiding in the same package in core must be approved too, see bug 197981.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-16 16:05 EST ------- Erm, help gkrellm already is in CVS for RHL-8 and RHL-9, but no devel dir and the import script chokes because it already is in CVS. What now?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |wtogami@redhat.com
------- Additional Comments From ville.skytta@iki.fi 2006-07-16 16:32 EST ------- Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a pointer to this report?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-17 03:20 EST ------- (In reply to comment #14)
Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a pointer to this report?
Done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197967
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-07-19 17:56 EST ------- Thanks!
Imported and build, closing.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/show_bug.cgi?id=197967
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From ville.skytta@iki.fi 2007-09-01 16:23 EST ------- Package Change Request ====================== Package Name: gkrellm New Branches: EL-5 Updated EPEL Owners: scop,jwrdegoede
As discussed in private mail with Hans.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: gkrellm - Multiple stacked system monitors in one process
https://bugzilla.redhat.com/show_bug.cgi?id=197967
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
------- Additional Comments From kevin@tummy.com 2007-09-03 14:40 EST ------- cvs done.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla