Bug 177105

Summary: Review Request: gnomeradio - Graphical FM-Tuner program
Product: [Fedora] Fedora Reporter: Dominik 'Rathann' Mierzejewski <dominik>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: j, mpeters, splinux, tcallawa
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-09-08 14:02:45 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    

Description Dominik 'Rathann' Mierzejewski 2006-01-06 12:45:24 UTC
Spec: http://rpm.greysector.net/extras/gnomeradio.spec
SRPM: http://rpm.greysector.net/extras/gnomeradio-1.6-2.src.rpm
Description:
A FM-Tuner program for GNOME.

Comment 1 Brian Pepple 2006-01-06 17:44:32 UTC
Here a couple of quick things I noticed that need to be corrected:

1. URL should be: http://www.wh-hms.uni-ulm.de/~mfcn/gnomeradio/
2. SOURCE should be:
http://www.wh-hms.uni-ulm.de/~mfcn/%{name}/packages/%{name}-%{version}.tar.gz
3. Your missing: Requires(post): scrollkeeper - refer to
http://fedoraproject.org/wiki/ScriptletSnippets#head-3c9f517f0cd4aaabb369a8805226d85dc2f02793
4. Desktop file is not correct: refer to
http://fedoraproject.org/wiki/PackagingGuidelines#head-37131c9c3cb4b69fdb1269f6e91fa9c413d2add1
5. Remove the %doc preceeding the help documents.
6. Missing Requires for GConf2: refer to
http://fedoraproject.org/wiki/ScriptletSnippets#head-ff64cd482595764f672082d5a3b83e1fc22962e8

I would suggest going over the packaging information on the Wiki, since many of
the problems with your spec can find the resolution there.
http://fedoraproject.org/wiki/Extras


Comment 2 Dominik 'Rathann' Mierzejewski 2006-01-06 18:13:52 UTC
Fixed all except 5. Why should I remove %doc?

Comment 3 Brian Pepple 2006-01-06 19:07:00 UTC
(In reply to comment #2)
> Fixed all except 5. Why should I remove %doc?

What benefit are you getting by keeping it?  It's not like your using RPM to
create a package-specific documentation directory during installation for these
files.  Also, I'm not aware of any other spec in FE that marks the help
documentation with the %doc.


Comment 4 Brian Pepple 2006-01-06 19:23:53 UTC
Oh, glancing at your most recent changes to the spec file, you have an error in
the %preun section.  You need to replace [NAME] with %{name}.

Comment 5 Frank Arnold 2006-01-07 00:21:13 UTC
(In reply to comment #2)
> Fixed all except 5. Why should I remove %doc?

"%doc preceeding the help documents" - Perhaps a misunderstanding?
The %doc tag was meant, not the %doc line before.

PackageReviewGuidelines:
- MUST: If a package includes something as %doc, it must not affect the runtime
of the application. To summarize: If it is in %doc, the program must run
properly if it is not present.

This is what I read out of it:
If help files are not present due to install with --excludedocs you will
hopefully get an error message when pressing a help button. Thus it affects the
runtime of your application.


Comment 6 Brian Pepple 2006-01-07 00:43:08 UTC
(In reply to comment #5)
> "%doc preceeding the help documents" - Perhaps a misunderstanding?
> The %doc tag was meant, not the %doc line before.

Correct.  I was referring to help documents (%{_datadir}/gnome/help/%{name}/),
not the README, AUTHORS, etc. line.  Speaking of which though, you also need to
add the COPYING file.

Comment 7 Michael A. Peters 2006-01-07 01:31:45 UTC
Just a suggestion - I would personally be a little more detailed on the
%description:

IE
%description
Gnomeradio is a FM-radio tuner for the GNOME desktop. It should work with every
FM tuner card that is supported by video4linux. Remote controls are supported
via LIRC-support. Gnomeradio can also record radio as a Wave or Ogg files.

-=-
That's from the homepage and modified a little bit.
And I think you need a line break every 80 characters or so (which I didn't do
above).

Comment 8 Dominik 'Rathann' Mierzejewski 2006-01-07 16:30:10 UTC
Fixed all, changed the release tag down to 1, because it doesn't exist in FE
yet. Thanks.

Comment 9 Michael Schwendt 2006-03-05 12:10:58 UTC
* Missing   BuildRequires gettext

* LIRC is also available in Extras (lirc-devel).

* rpmlint  /home/qa/tmp/rpm/RPMS/gnomeradio-1.6-1.i386.rpm
W: gnomeradio conffile-without-noreplace-flag /etc/gconf/schemas/gnomeradio.sche
mas

It should not be marked %config.

* /usr/share/pixmaps/radio.png

This is pollution of the global pixmaps namespace and should be avoided.
The name "radio.png" is way too generic. The package stores "gnomeradio.png"
in the same directory, which creates a slightly better namespace. Suggest
renaming radio.png, also in the .desktop file.

* $ sudo rpm -ivh -p gnomeradio-1.6-1.i386.rpm 
> Preparing...                ########################################### [100%]
>    1:gnomeradio             ########################################### [100%]
> gconfd-2: no process killed

Killing gconfd is no longer necessary in FC5, and in case it would be
necessary, make it shut up by redirecting its output. It is killed more
than once in the scriptlets, which is a guarantee for this warning.

* BuildRequires: gtk2-devel libgnomeui-devel

Only if you're into "optimising away" redundant build requirements,
notice that libgnomeui-devel requires gtk2-devel and glib2-devel already.
Else add a comment to make it explicit that _your_ package also depends
on the glib API directly (glib2-devel).


Comment 10 Dominik 'Rathann' Mierzejewski 2006-04-02 20:05:31 UTC
Fixed.

Comment 11 Michael Schwendt 2006-04-04 21:51:32 UTC
APPROVED

If you don't have a Fedora Extras account yet, continue at step 10
here,

  http://fedoraproject.org/wiki/Extras/Contributors

and I'll sponsor you.


Comment 12 Brian Pepple 2006-04-08 23:53:41 UTC
*** Bug 188395 has been marked as a duplicate of this bug. ***

Comment 13 Michael Schwendt 2006-05-13 23:44:55 UTC
*ping*

Comment 14 Michael Schwendt 2006-07-06 09:03:34 UTC
Okay, everyone who reads this please listen:

Hereby I announce that I withdraw my approval from 2006-04-04
and move this ticket back into the FE-NEW queue. Too much time has
passed without action from the packager. Further, another package
submission from a different packager has been rejected (bug 188395),
but the software is still not in Fedora Extras after three months.
This simply doesn't work and is a major hindrance, IMO. An approved
package must not block other submissions for months. And it becomes
an extra burden for me as a reviewer.


Comment 15 Dominik 'Rathann' Mierzejewski 2006-08-01 16:49:20 UTC
Yes, you're totally right and I'm very sorry about the delay. It won't happen
again. I've just jumped through the hoops of getting an account and pushed my
first approved package (sysconftool). I promise to be more responsive from now on.

Comment 16 Jason Tibbitts 2006-08-13 01:34:18 UTC
Is the package back in comment #1 the one that should be reviewed?

Comment 18 Jason Tibbitts 2006-09-06 20:25:25 UTC
OK, this should be easy because of all of the other review work.  

It looks like your BuildRequires: pkgconfig is not strictly necessary on FC6 at
least, but i doesn't harm anything.

rpmlint only says:
W: gnomeradio non-conffile-in-etc /etc/gconf/schemas/gnomeradio.schemas
which in this case is bogus.

It looks like /usr/share/omf is unowned; I don't see anything in the dependency
chain that would create it.  It seems that a whole pile of packages own it
already, so it seems the expectation is that you should own it as well.

* source files match upstream:
   07b9d511f79e38f114af51cc7bfc014a  gnomeradio-1.6.tar.gz
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
O BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* debuginfo package looks complete.
* rpmlint has only ignorable complaints.
* final provides and requires are sane:
   gnomeradio = 1.6-2.fc6
  =
   /bin/sh
   GConf2
   libICE.so.6()(64bit)
   libORBit-2.so.0()(64bit)
   libSM.so.6()(64bit)
   libX11.so.6()(64bit)
   libart_lgpl_2.so.2()(64bit)
   libatk-1.0.so.0()(64bit)
   libbonobo-2.so.0()(64bit)
   libbonobo-activation.so.4()(64bit)
   libbonoboui-2.so.0()(64bit)
   libcairo.so.2()(64bit)
   libgconf-2.so.4()(64bit)
   libgdk-x11-2.0.so.0()(64bit)
   libgdk_pixbuf-2.0.so.0()(64bit)
   libglib-2.0.so.0()(64bit)
   libgmodule-2.0.so.0()(64bit)
   libgnome-2.so.0()(64bit)
   libgnome-keyring.so.0()(64bit)
   libgnomecanvas-2.so.0()(64bit)
   libgnomeui-2.so.0()(64bit)
   libgnomevfs-2.so.0()(64bit)
   libgobject-2.0.so.0()(64bit)
   libgthread-2.0.so.0()(64bit)
   libgtk-x11-2.0.so.0()(64bit)
   liblirc_client.so.0()(64bit)
   libpango-1.0.so.0()(64bit)
   libpangocairo-1.0.so.0()(64bit)
   libpangoft2-1.0.so.0()(64bit)
   libpopt.so.0()(64bit)
   libxml2.so.2()(64bit)
   libz.so.1()(64bit)
   scrollkeeper
* %check is not present; no test suite upstream.
* no shared libraries are added to the regular linker search paths.
* package is not relocatable.
? owns the directories it creates (/usr/share/omf?)
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets present are OK (gconf schema installation, scrollkeeper-update)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* GUI app; desktop file looks to be installed properly.

So it's just the /usr/share/omf thing that needs fixing or clarification.

Comment 19 Dominik 'Rathann' Mierzejewski 2006-09-07 12:57:55 UTC
I don't think it is appropriate for this package to own /usr/share/omf. Maybe
scrollkeeper should own it.

Comment 20 Jason Tibbitts 2006-09-07 20:16:36 UTC
Looks like scrollkeeper has been fixed in rawhide.  That leaves FC5, but it's at
best a minor issue and I don't think it should block this package.

APPROVED

Comment 21 Dominik 'Rathann' Mierzejewski 2006-09-08 14:02:45 UTC
Thanks for the review. I removed the pkgconfig BR, because it's redundant on FC5
as well. Package imported and built for devel, FC-5 branch requested.