Bug 524605 - Review Request: gtrayicon - Generic tray icon for GNOME
Summary: Review Request: gtrayicon - Generic tray icon for GNOME
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dominic Hopf
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-21 13:12 UTC by Christoph Wickert
Modified: 2009-11-04 12:24 UTC (History)
4 users (show)

Fixed In Version: 1.1-2.fc10
Clone Of:
Environment:
Last Closed: 2009-11-04 12:22:03 UTC
Type: ---
Embargoed:
dmaphy: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Christoph Wickert 2009-09-21 13:12:40 UTC
Spec URL: http://cwickert.fedorapeople.org/review/gtrayicon.spec
SRPM URL: http://cwickert.fedorapeople.org/review/gtrayicon-1.1-1.fc12.src.rpm
Description: Gtrayicon is a generic tray icon for GNOME. It defines two actions: activate and deactivate, that are alternated by clicking in the tray icon. You choose what commands to run on each action. You can also define each action's icon and a custom menu that is shown on left click. A sample menu file is included with the package. Check /usr/share/doc/gtrayicon-1.1/sample.menu for more info.

Comment 1 Martin Gieseking 2009-09-22 07:29:48 UTC
Christoph, you should avoid compressing the man pages manually as the compression format depends on the target system and rpm applies it automatically. I'd suggest to modify the %build and %install section like this:

%build
make %{?_smp_mflags} CFLAGS="%{optflags}"


%install
rm -rf %{buildroot}
make install DESTDIR=%{buildroot} INSTALL='install -p'
install -Dpm0644 Debian/%{name}.1 %{buildroot}/%{_mandir}/man1/%{name}.1

Comment 2 Martin Gieseking 2009-09-22 07:32:53 UTC
Oh, and I forgot to mention the %files section:
%{_mandir}/man1/*

:)

Comment 3 Martin Gieseking 2009-09-22 07:41:23 UTC
The licensing is also a bit unclear:
- LICENSE contains GPLv3
- gtrayicon.glade refers to GPLv3+
- the SVG files link to GPLv2:
  <cc:License rdf:about="http://creativecommons.org/licenses/GPL/2.0/">

Comment 4 Christoph Wickert 2009-09-22 08:33:23 UTC
(In reply to comment #2)
> Oh, and I forgot to mention the %files section:
> %{_mandir}/man1/*

I understand the rest of your points but not this one. What's the benefit of changing the line?

Comment 5 Martin Gieseking 2009-09-22 08:46:14 UTC
Maybe %{_mandir}/man1/* is a bit too general, %{_mandir}/man1/%{name}.1* is probably a better choice. 
I just wanted to ensure that all dependencies on a particular compression format (including the suffix) are avoided. The above variants will even work on targets with uncompressed man pages.

Comment 6 Christoph Wickert 2009-09-22 23:39:01 UTC
Do we have such targets in Fedora? Do we have any manpages not gzipped?

Comment 7 Martin Gieseking 2009-09-23 11:58:18 UTC
I don't know. :)
But that's the point. I don't like to make too many assumptions if not necessary. If Fedora decided to change the compression format, the spec file would have to be adapted. Why not just avoid that by changing a few characters? 
However, I'm still familiarizing with Fedora packaging. If you say the explicit .gz suffix is fine then it is. To me at first glance, it's an inconsistency because the automatic compression is a transparent action and one should not rely on specific results.

Comment 8 Christoph Wickert 2009-10-11 09:20:32 UTC
Sorry, this slipped off my radar.

I agree that what you say makes some sense. However I don't see a reason to change the spec because *if* one day Fedora really decided to switch to another compression method for manpages, this would require a mass rebuild anyway. Enough time to change the spec when really needed and not based on vague assumptions. In several years of using Linux I have never seen manpages *not* gzipped. My primary objective is not accidentally packaging uncompressed or even unwanted files, e.g. due to a bad Makefile.

If you really think there should be no prefixes for manpages, please bring this suggestion up to the packaging committee on fedora-packaging-list.

Comment 9 Christoph Wickert 2009-10-11 09:23:35 UTC
(In reply to comment #3)
> The licensing is also a bit unclear:
> - LICENSE contains GPLv3
> - gtrayicon.glade refers to GPLv3+

results in GPLv3+

> - the SVG files link to GPLv2:

Not sure if this is really intended, but if, this would result in
"License: GPLv2 and GPLv3+" in the spec. Thanks for pointing this out.

Comment 10 Martin Gieseking 2009-10-12 13:57:30 UTC
(In reply to comment #8)
> If you really think there should be no prefixes for manpages, please bring this
> suggestion up to the packaging committee on fedora-packaging-list.  

No, it's not that important to me and I think it's also some kind of personal preference. As far as I noticed, a lot of Fedora packagers seem to prefer omitting the suffix, though.

Comment 11 Dominic Hopf 2009-10-17 12:22:18 UTC
$ rpmlint gtrayicon.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint gtrayicon-1.1-1.fc12.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint gtrayicon-1.1-1.fc10.x86_64.rpm gtrayicon-debuginfo-1.1-1.fc10.x86_64.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
NOT OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     the installed binary is not linked against any library
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there isn't any library installed with this package
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
NOT OK: contains desktop file if it is a GUI application
     this package actually provides GUI, but there is no desktop-file
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     a localization is not available for this package
OK: package builds in mock
OK: package builds into binary rpms for all supported architectures
OK: program runs
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required

Please update the License-Filed in specfile accordingly to what you wrote before
to GPLv3+ and contact upstream regarding the license issue in the *.svg-files.
There also is no licensing hint in the sourcefile (gtrayicon.c) itself, so you may want to suggest upstream to fix that.

A desktop-file seems to be missing. This obviously is a GUI application, how is
it intended to be started without an item in a menu? If necessary, please add
the desktop-file.

As Martin already said, since RPM applies compression for the manpage itself, you also should consider to apply his suggestion to modify the %build and %install section.

Once these three issues are fixed or clarified I will approve this package.

Comment 12 Christoph Wickert 2009-10-17 13:23:28 UTC
(In reply to comment #11)

> Please update the License-Filed in specfile accordingly to what you wrote
> before 
> to GPLv3+ and contact upstream regarding the license issue in the *.svg-files.

I have contacted the author of the program but got no reply so far. I think the licensing of the icons is different than the one from the program, because they were done by two different authors, who AFAIK published them as GPLv2+. So we could use GPLv3+ in the spec, but I'll wait for further clarification from upstream first.

> There also is no licensing hint in the sourcefile (gtrayicon.c) itself, so you
> may want to suggest upstream to fix that.

Will do.

> A desktop-file seems to be missing. This obviously is a GUI application, how is
> it intended to be started without an item in a menu? If necessary, please add
> the desktop-file.

A desktop file is useless because the command to run gtraycon needs to be customized. Just calling grayicon doesn't work, you need to specify the commands for 'activate' and 'deactivate' and optionally a custom menu file. The resulting commandline should be added in the session-properties to autostart the trayicon, so there really is no need for a desktop file.

> As Martin already said, since RPM applies compression for the manpage itself,
> you also should consider to apply his suggestion to modify the %build and
> %install section.

Agreed. I will remove the compression statement and the suffix on the file. Nevertheless I think that Martin should bring up the suffix topic on fedora-packaging because it really makes sense what he said.

Comment 13 Christoph Wickert 2009-10-17 18:04:21 UTC
Reply from upstream: "Sure, I want to stick to GPLv3+, I just used gnome artwork because I found it was GPL too, but I messed with versions."

So GPLv3+ for the whole thing is correct. I have also fixed the other issues (except from the desktop file).

SPEC: http://cwickert.fedorapeople.org/review/gtrayicon.spec
SRPM: http://cwickert.fedorapeople.org/review/gtrayicon-1.1-2.fc12.src.rpm

Comment 14 Dominic Hopf 2009-10-17 18:24:18 UTC
$ rpmlint gtrayicon.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint gtrayicon-1.1-2.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint gtrayicon-1.1-2.fc11.x86_64.rpm gtrayicon-debuginfo-1.1-2.fc11.x86_64.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     the installed binary is not linked against any library
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there isn't any library installed with this package
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
OK: contains desktop file if it is a GUI application
     This is a GUI program whichs needs to be started with arguments from command
     line
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     a localization is not available for this package
OK: package builds in mock
OK: package builds into binary rpms for all supported architectures
OK: program runs
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required

APPROVED.

Comment 15 Christoph Wickert 2009-10-17 18:28:19 UTC
Thanks for the review!

New Package CVS Request
=======================
Package Name: gtrayicon
Short Description: Generic tray icon for GNOME
Owners: cwickert
Branches: F-10 F-11 F-12
InitialCC:

Comment 16 Kevin Fenzi 2009-10-17 20:40:45 UTC
cvs done.

Comment 17 Fedora Update System 2009-10-17 21:52:18 UTC
gtrayicon-1.1-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/gtrayicon-1.1-2.fc12

Comment 18 Fedora Update System 2009-10-17 21:52:33 UTC
gtrayicon-1.1-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/gtrayicon-1.1-2.fc11

Comment 19 Fedora Update System 2009-10-17 21:52:49 UTC
gtrayicon-1.1-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gtrayicon-1.1-2.fc10

Comment 20 Fedora Update System 2009-10-21 00:36:29 UTC
gtrayicon-1.1-2.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update gtrayicon'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10565

Comment 21 Fedora Update System 2009-10-21 00:39:27 UTC
gtrayicon-1.1-2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update gtrayicon'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-10587

Comment 22 Fedora Update System 2009-11-04 12:21:57 UTC
gtrayicon-1.1-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2009-11-04 12:24:37 UTC
gtrayicon-1.1-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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