[Bug 583102] New: Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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

Summary: Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

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

Summary: Review Request: radiotray - Radio Tray is a streaming
                    player for listening to online radios
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

Spec URL: http://jfsaucier.fedorapeople.org/packages/radiotray.spec
SRPM URL:
http://jfsaucier.fedorapeople.org/packages/radiotray-0.5.1-1.fc12.src.rpm
Description: 
Radio Tray is an online radio streaming player that runs on a Linux system
tray. Its goal is to have the minimum interface possible, making it very
straightforward to use. Radio Tray is not a full featured music player,
there are plenty of excellent music players already. However, there was a
need for a simple application with minimal interface just to listen to online
radios. And that's the sole purpose of Radio Tray.

rpmlint give no warning/error on spec, srpm and rpm.

The koji scratch build are all fine for F-11, F-12, F-13 and rawhide :
- http://koji.fedoraproject.org/koji/taskinfo?taskID=2115316
- http://koji.fedoraproject.org/koji/taskinfo?taskID=2115320
- http://koji.fedoraproject.org/koji/taskinfo?taskID=2115329
- http://koji.fedoraproject.org/koji/taskinfo?taskID=2115333

Here is what the two patches do :

radiotray-0.5.1-fix-setup.patch : remove some redundant files

radiotray-0.5.1-fix-desktop-file.patch : fix the syntax of the desktop
file to comply with the guidelines.

Thank you!

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Ankur Sinha  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

--- Comment #1 from Ankur Sinha  2010-05-07 01:13:13 EDT ---
hey,

a quick look.

## The python_sitelib definition could be changed to

http://fedoraproject.org/wiki/Packaging/Python#Macros

## The build section could use

CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build

This way the correct flags are used.

## line 37 could use %{__python} instead of python (optional)

## %{python_sitelib}/radiotray-0.5.1-py?.?.egg-info would help not break the
package in case of py version changes?

No major issues, I'll do a complete review within the coming week hopefully.

regards,
Ankur

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #2 from Jean-Francois Saucier  2010-05-07 10:16:26 EDT ---
Spec URL: http://jfsaucier.fedorapeople.org/packages/radiotray.spec
SRPM URL:
http://jfsaucier.fedorapeople.org/packages/radiotray-0.5.1-2.fc12.src.rpm

Thank you for your suggestions. rpmlint is still silent after the changes and
build fine in mock.
> ## The python_sitelib definition could be changed to

Done, I did not notice this change since F-13, thanks for pointing this to me!
> ## The build section could use
> CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build

Done.
> ## line 37 could use %{__python} instead of python (optional)

Done.
> ## %{python_sitelib}/radiotray-0.5.1-py?.?.egg-info would help not break the
> package in case of py version changes?

Done.

Thanks for pointing out some python stuff. I don't have many experience
packaging python applications and with the new multiple python version feature
in F-13, I think it's important to make these changes.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #3 from Ankur Sinha  2010-05-10 13:43:34 EDT ---
review:

+ OK
? issue
- NA

+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
? License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
+ Sources match upstream md5sum:
[Ankur@localhost SPECS]$ md5sum radiotray-0.5.1.tar.gz 
9811f8145108784e8e515b66fdaa6f05  radiotray-0.5.1.tar.gz
[Ankur@localhost SPECS]$ md5sum ../SOURCES/radiotray-0.5.1.tar.gz 
9811f8145108784e8e515b66fdaa6f05  ../SOURCES/radiotray-0.5.1.tar.gz

- Package needs ExcludeArch
+ BuildRequires correct
+ Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
+ Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

+ Package is a GUI app and has a .desktop file

+ Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
+ No rpmlint output.

NOT CHECKED YET - final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =;
rpm -qp --requires $i; echo; done

SHOULD Items:

+ Should build in mock.: builds in koji
- Should build on all supported archs
+ Should function as described.
- Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
+ Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. please check the license, the COPYING and source files feature a GPLv1+
2. please comment the spec file and add reasons for patching in there too.
3. why are explicit requires used in the spec? Please add comments justifying
them

http://fedoraproject.org/wiki/PackagingGuidelines#Explicit_Requires

Please correct the above, I'll complete the review once they're taken care of.

regards,
Ankur

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #4 from Jean-Francois Saucier  2010-05-13 09:14:36 EDT ---
I have a question before posting a new package :
> please check the license, the COPYING and source files feature a GPLv1+

Done. You are right, the website mention GPL and I assume GPLv3... It's clearly
a GPLv1+ in the COPYING file.
> please comment the spec file and add reasons for patching in there too.

Done. I added the comments to patches and 1-2 other places in the SPEC file.
> why are explicit requires used in the spec? Please add comments justifying them

This is my question. I don't see the explicit requires in my SPEC file. I think
I don't have any but maybe it's something I don't know. Can you enlight me on
this?

Thanks for the review.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #5 from Ankur Sinha  2010-05-13 09:24:57 EDT ---
(In reply to comment #4)
hey,
> > why are explicit requires used in the spec? Please add comments justifying them
>
> This is my question. I don't see the explicit requires in my SPEC file. I think
> I don't have any but maybe it's something I don't know. Can you enlight me on
> this?
>
> Thanks for the review.

That would be line 18.

Requires:       gstreamer-python pygtk2 pygobject2 python-lxml python-inotify

regards,
Ankur

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #6 from Michael Schwendt  2010-05-13 09:59:46 EDT ---
> It's clearly a GPLv1+ in the COPYING file.

The COPYING file contains GPL version 1.

The 1+ comes from the .py file headers.

The Fedora license tag for that is GPL+ not GPLv1+.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #7 from Jean-Francois Saucier  2010-05-16 16:08:22 EDT ---
I removed the pygobject2 from the Requires because pygtk2 require it
automatically. Is it what you mean by explicit requires? If yes, I am ready to
provide a new SPEC and SRPM.

Excuse me if it's a dump question but I always though that explicit require was
more like : libfubar >= 0:1.2.3-7.

Thanks!

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #8 from Michael Schwendt  2010-05-19 11:13:53 EDT ---
The guidelines just ask you to add a comment in the spec file and explain why
explicit dependencies are needed. In this case they are needed, because there
are no automatic dependencies for the needed Python modules.

[...]

With regard to eliminating redundant Requires (e.g. pygobject2 <-- pygtk2,
although radiotray directly imports the gobject module, too), it's not
necessary to trim such Requires, and in other cases it may even bear a risk.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #9 from Ankur Sinha  2010-05-21 03:15:08 EDT ---
(In reply to comment #8)
> The guidelines just ask you to add a comment in the spec file and explain why
> explicit dependencies are needed. In this case they are needed, because there
> are no automatic dependencies for the needed Python modules.
>
> [...]
>
> With regard to eliminating redundant Requires (e.g. pygobject2 <-- pygtk2,
> although radiotray directly imports the gobject module, too), it's not
> necessary to trim such Requires, and in other cases it may even bear a risk.

Thanks for pitching in Michael :)

Jean, please submit the updated spec and srpm and we can get this one approved
quick. :)

Ankur

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #10 from Jean-Francois Saucier  2010-05-25 11:00:38 EDT ---
Spec URL: http://jfsaucier.fedorapeople.org/packages/radiotray.spec
SRPM URL:
http://jfsaucier.fedorapeople.org/packages/radiotray-0.5.1-3.fc12.src.rpm

Hi,

Thanks for the review and pardon me for taking so much time to post an update,
I was in training for the entire week.

Everything should be fixed now! rpmlint is still silent after the changes and
the package build fine in mock.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Ankur Sinha  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #11 from Ankur Sinha  2010-05-29 03:08:43 EDT ---
hey,

[ankur@mether SPECS]$ rpmlint radiotray.spec
/var/lib/mock/fedora-rawhide-i386/result/*.rpm
radiotray.spec: W: invalid-url Source0:
http://downloads.sourceforge.net/radiotray/radiotray-0.5.1.tar.gz <urlopen
error timed out>
radiotray.noarch: W: no-manual-page-for-binary radiotray
radiotray.src: W: invalid-url Source0:
http://downloads.sourceforge.net/radiotray/radiotray-0.5.1.tar.gz <urlopen
error timed out>
2 packages and 1 specfiles checked; 0 errors, 3 warnings.

Only one worth looking at is the second one. I'd suggest writing a man page for
the package.

You could also try to make this work on python3.

Apart from this, it looks good :)

XXX APPROVED XXX

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #13 from Ankur Sinha  2010-05-29 03:18:34 EDT ---
hey,

[ankur@ SPECS]$ rpmlint radiotray.spec
/var/lib/mock/fedora-rawhide-i386/result/*.rpm
radiotray.spec: W: invalid-url Source0:
http://downloads.sourceforge.net/radiotray/radiotray-0.5.1.tar.gz <urlopen
error timed out>
radiotray.noarch: W: no-manual-page-for-binary radiotray
radiotray.src: W: invalid-url Source0:
http://downloads.sourceforge.net/radiotray/radiotray-0.5.1.tar.gz <urlopen
error timed out>
2 packages and 1 specfiles checked; 0 errors, 3 warnings.

I'd advise you to write up a small man page if you can. Also, have a look if
you can append python3 support to the spec.

Rest looks cool.

XXX APPROVED XXX

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Jean-Francois Saucier  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #14 from Jean-Francois Saucier  2010-05-31 08:45:38 EDT ---
Thank you for the review! I will try to work with upstream for the things you
mention in the near future.

New Package CVS Request
=======================
Package Name: radiotray
Short Description: Radio Tray is a streaming player for listening to online
radios
Owners: jfsaucier
Branches: F-11 F-12 F-13
InitialCC:

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #15 from Kevin Fenzi  2010-05-31 15:19:35 EDT ---
CVS done (by process-cvs-requests.py).

We no longer allow f11 branches with the release of f13.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #16 from Fedora Update System  2010-06-02 19:21:33 EDT ---
radiotray-0.5.1-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/radiotray-0.5.1-3.fc13

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #17 from Fedora Update System  2010-06-02 19:22:55 EDT ---
radiotray-0.5.1-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/radiotray-0.5.1-3.fc12

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |ON_QA

--- Comment #18 from Fedora Update System  2010-06-03 14:04:52 EDT ---
radiotray-0.5.1-3.fc13 has been pushed to the Fedora 13 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 radiotray'.  You can provide
feedback for this update here:
http://admin.fedoraproject.org/updates/radiotray-0.5.1-3.fc13

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #19 from Fedora Update System  2010-06-03 14:13:26 EDT ---
radiotray-0.5.1-3.fc12 has been pushed to the Fedora 12 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 radiotray'.  You can provide
feedback for this update here:
http://admin.fedoraproject.org/updates/radiotray-0.5.1-3.fc12

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #20 from Fedora Update System  2010-06-10 15:18:36 EDT ---
radiotray-0.5.1-3.fc13 has been pushed to the Fedora 13 stable repository.  If
problems still persist, please make note of it in this bug report.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
   Fixed In Version|                            |radiotray-0.5.1-3.fc13
         Resolution|                            |ERRATA

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

--- Comment #21 from Fedora Update System  2010-06-10 15:19:42 EDT ---
radiotray-0.5.1-3.fc12 has been pushed to the Fedora 12 stable repository.  If
problems still persist, please make note of it in this bug report.

[Bug 583102] Review Request: radiotray - Radio Tray is a streaming player for listening to online radios

[Red Hat Bugzilla]

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=583102

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Fixed In Version|radiotray-0.5.1-3.fc13      |radiotray-0.5.1-3.fc12