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
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}).
(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.
(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.
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> 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
- "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
Created attachment 132478 [details] Suggested patch for findings in comment 5
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).
(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> 2.2.9-6 - Various specfile improvements by Ville Skyttä (ville.skytta) - 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
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.
(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> 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
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.
(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.
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?
Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a pointer to this report?
(In reply to comment #14) > Maybe ask for an empty devel branch in CVSSyncNeeded in Wiki, including a > pointer to this report? Done.
Thanks! Imported and build, closing.
Package Change Request ====================== Package Name: gkrellm New Branches: EL-5 Updated EPEL Owners: scop,jwrdegoede As discussed in private mail with Hans.
cvs done.
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.
Git done (by process-git-requests).