[Bug 513797] Review Request: gnome-applet-cpufire - GNOME panel applet showing the CPU load as a fire

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=513797


Edwin ten Brink <fedora at tenbrink-bekkers.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |needinfo?(fedora at christoph-
                   |                            |wickert.de)




--- Comment #7 from Edwin ten Brink <fedora at tenbrink-bekkers.nl>  2009-07-31 03:31:22 EDT ---
(In reply to comment #6)
> Thanks for this detailed review. I really like people reviewing carefully and
> I'm glad I sponsored you.

You're welcome. I appreciate your sponsorship.

> (In reply to comment #5)
> > In summary:
> > - rpmlint warning to be fixed
> 
> done

OK: rpmlint is quiet now.

> > - license does not seem to match
> 
> The package includes a copy of GPLv2, but it's unclear wether is "GPLv2 only"
> or GPLv2 "or any later version". The headers of the source contain no license
> block ether, so I mailed the author and his reply was that it's "clearly
> GPLv2+". Quote: "Das steht aber klar im COPYRIGHT: GPLv2+"
> In line 298/299 of COPYING also "or any later version" is mentioned, so GPLv2+
> definitely is correct.

Line 298/299 is not part of the license, but merely an example of how to
include GPLv2(+) in your programs. As most people choose to literally take the
text (which is not a bad idea), this text can usually be found in headers
and/or source files. However, the author did not do so. Stricly legally,
therefore only GPLv2 is applicable, IMO. It would be better if the author would
include the text and actions as mentioned in GPLv2 (lines 282-311), perhaps you
could communicate that to upstream.
OK: If the author indeed confirmed that GPLv2+ is applicable, then you can
leave it as it is (but please contact the author).

> > - comment out libgnomeui-devel in BuildRequires including comment as to why
> 
> Why should I comment out libgnomeui? It's needed, cpufire_applet.c has
> ...
> #include <libgnomeui/libgnomeui.h>
> ...

NOT OK: I agree that you need libgnomeui-devel, but libgnomeui-devel gets
pulled in by gnome-panel-devel. Again, as per
http://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires, it should
be removed. We implicitly agreed on this during the review of
gnome-applet-bubblemon (bug 497525):
http://cvs.fedoraproject.org/viewvc/rpms/gnome-applet-bubblemon/devel/gnome-applet-bubblemon.spec?revision=1.1&view=markup

> > - either add comment to, or remove BR GConf2 commented out
> 
> During configure you'll see:
> ...
> Using config source xml:merged:/etc/gconf/gconf.xml.defaults for schema
> installation
> Using $(sysconfdir)/gconf/schemas as install directory for schema files
> ...
> 
> Only GConf2 is needed, not GConf2-devel. As GConf is already pulled in by
> several other BuildRequires I removed it.

OK.

> > - explain why you need gnome-panel-devel >= 2.6
> 
> ...
> #include <panel-applet.h>
> #include <panel-applet-gconf.h>
> ...
> $ rpm -qf /usr/include/panel-2.0/panel-applet.h
> /usr/include/panel-2.0/panel-applet-gconf.h 
> gnome-panel-devel-2.26.3-1.fc11.x86_64
> gnome-panel-devel-2.26.3-1.fc11.x86_64

NOT OK: I'm sorry the comment appears to be unclear. I know you depend on
gnome-panel-devel, but why did you state version >= 2.6. This requirement on
the version is not documented in the tarball, and AFAICS, is not checked by the
configure script. If you indeed require >= 2.6, then this version is also 'very
old'. Fedora 7 included 2.18.3-1. As per
http://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires, you
should drop te version requirement.

> > - own %{_datadir}/omf/cpufire/
> 
> fixed

OK.

> > - optionally: include the empty files MAINTAINERS, NEWS and TODO  
> 
> I agree with you that including these 0kb files is easier, but common practice
> in Fedora is not to include them, otherwise rpmlint will complain again. I will
> add them as soon as they have content.

OK.



Open issues:
- superfluous dependency on libgnomeui-devel in BuildRequires
- versioned dependency on gnome-panel-devel >= 2.6 in BuildRequires
- contact the upstream author to see if he would include the text and actions
as mentioned in GPLv2 (lines 282-311) in the next release

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.