Bug 197967 - Review Request: gkrellm - Multiple stacked system monitors in one process
Review Request: gkrellm - Multiple stacked system monitors in one process
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 197981
  Show dependency treegraph
 
Reported: 2006-07-07 14:57 EDT by Hans de Goede
Modified: 2014-08-12 15:16 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-19 17:56:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑cvs+


Attachments (Terms of Use)
New specfile fixing discussed issues (13.20 KB, text/plain)
2006-07-15 02:12 EDT, Hans de Goede
no flags Details
Suggested patch for findings in comment 5 (3.30 KB, patch)
2006-07-15 05:09 EDT, Ville Skyttä
no flags Details | Diff

  None (edit)
Description Hans de Goede 2006-07-07 14:57:01 EDT
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
Comment 1 Ville Skyttä 2006-07-10 15:57:35 EDT
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}).
Comment 2 Hans de Goede 2006-07-14 14:57:17 EDT
(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.
Comment 3 Ville Skyttä 2006-07-14 15:29:45 EDT
(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.
Comment 4 Hans de Goede 2006-07-15 02:12:26 EDT
Created attachment 132475 [details]
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
Comment 5 Ville Skyttä 2006-07-15 05:07:33 EDT
- "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
Comment 6 Ville Skyttä 2006-07-15 05:09:20 EDT
Created attachment 132478 [details]
Suggested patch for findings in comment 5
Comment 7 Ville Skyttä 2006-07-15 05:13:29 EDT
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).
Comment 8 Hans de Goede 2006-07-15 07:01:43 EDT
(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

Comment 9 Ville Skyttä 2006-07-16 10:55:49 EDT
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.
Comment 10 Hans de Goede 2006-07-16 14:41:00 EDT
(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

Comment 11 Ville Skyttä 2006-07-16 15:58:48 EDT
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.
Comment 12 Hans de Goede 2006-07-16 16:01:35 EDT
(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.
Comment 13 Hans de Goede 2006-07-16 16:05:00 EDT
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?
Comment 14 Ville Skyttä 2006-07-16 16:32:11 EDT
Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a
pointer to this report?
Comment 15 Hans de Goede 2006-07-17 03:20:42 EDT
(In reply to comment #14)
> Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a
> pointer to this report?

Done.
Comment 16 Hans de Goede 2006-07-19 17:56:48 EDT
Thanks!

Imported and build, closing.
Comment 17 Ville Skyttä 2007-09-01 16:23:14 EDT
Package Change Request
======================
Package Name: gkrellm
New Branches: EL-5
Updated EPEL Owners: scop,jwrdegoede

As discussed in private mail with Hans.
Comment 18 Kevin Fenzi 2007-09-03 14:40:39 EDT
cvs done. 
Comment 19 Adam Goode 2014-08-12 14:54:30 EDT
Package Change Request
======================
Package Name: gkrellm
New Branches: epel7
Owners: agoode

I would like to maintain this for epel7. I tried contacting the el6 maintainer with no response. See bug #1118085.
Comment 20 Jon Ciesla 2014-08-12 15:16:04 EDT
Git done (by process-git-requests).

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