Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 197967
Summary: | Review Request: gkrellm - Multiple stacked system monitors in one process | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Hans de Goede <hdegoede> | ||||||
Component: | Package Review | Assignee: | Ville Skyttä <scop> | ||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | adam, karsten, wtogami | ||||||
Target Milestone: | --- | Flags: | gwync:
fedora-cvs+
|
||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2006-07-19 21:56:48 UTC | Type: | --- | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Bug Depends On: | |||||||||
Bug Blocks: | 163779, 197981 | ||||||||
Attachments: |
|
Description
Hans de Goede
2006-07-07 18:57:01 UTC
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). |