Bug 197967

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).