Bug 513733 - Review Request: entertainer - A simple media center
Summary: Review Request: entertainer - A simple media center
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-25 00:55 UTC by Julian Aloofi
Modified: 2009-08-15 21:43 UTC (History)
5 users (show)

Fixed In Version: 0.4.2-5.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-15 21:42:01 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
The mock traceback for the rawhide build (614 bytes, text/plain)
2009-07-25 15:48 UTC, Julian Aloofi
no flags Details

Description Julian Aloofi 2009-07-25 00:55:56 UTC
Spec URL: http://julian.fedorapeople.org/entertainer/entertainer.spec
SRPM URL: http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-1.fc11.src.rpm
Description: Entertainer is a simple and easy-to-use media center solution for
Gnome and XFCE desktop environments. It uses the gstreamer framework
for multimedia playback and is based on clutter.

Comment 1 Susi Lehtola 2009-07-25 09:30:52 UTC
- Package does not build in mock. setup.py tries to run gtk-update-icon-cache, which is missing in the buildroot. To fix this, remove the call by running
 sed -i /gtk-update-icon-cache/d setup.py
in the %prep phase.

- Incorrect(?) dependencies: according to docs/DEPENDENCIES
Requires: pyclutter-cairo should be pycairo
Requires: python-storm-sqlite should be python-storm

- Unnecessary dependencies
Requires: pyclutter-gtk
Requires: pyclutter-gst
Drop these if you don't have a valid reason to have them.

- Missing Requires: hicolor-icon-theme for dir ownership. Please add also the macros to update the icon cache
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache


**

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. OK
- Please add an empty line (or two as you seem to use) between %build and %install.

MUST: The package must be named according to the Package Naming Guidelines. OK
- Not a (pure) python module so naming is OK.

MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. OK
MUST: Optflags are used and time stamps preserved. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK


MUST: Files only listed once in %files listings. NEEDSWORK

- For consistency, use
 %{_bindir}/entertainer*
instead of
 %{_bindir}/%{name}*
(You could also list the four files explicitly.)

- Change
 %{python_sitelib}/entertainerlib*
to
 %{python_sitelib}/entertainerlib/
and
 %{python_sitelib}/Entertainer-0.4-py2.6.egg-info
to
 %{python_sitelib}/Entertainer-*.egg-info
as you otherwise won't be able to build the spec file in Fedora 10 which has Python 2.5.

- Change
 %{_datadir}/entertainer*
to
 %{_datadir}/entertainer/

- Change
 %{_datadir}/icons/hicolor/*
to
 %{_datadir}/icons/hicolor/*/apps/entertainer.png
(You don't want to own system directories.)


MUST: Debuginfo package is complete. N/A
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK

MUST: Large documentation files must go in a -doc subpackage. N/A
- developer_documentation.pdf is 237K, which is OK compared to the rest of the package.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Desktop files are installed properly. OK
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Comment 2 Julian Aloofi 2009-07-25 15:46:54 UTC
(In reply to comment #1)
> - Package does not build in mock. setup.py tries to run gtk-update-icon-cache,
> which is missing in the buildroot. To fix this, remove the call by running
>  sed -i /gtk-update-icon-cache/d setup.py
> in the %prep phase.
Thanks, I already wondered what causes that problem. Builds in mock for F10 and F11 now, but fails in rawhide. I attached the traceback. Looks like the return of this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=442289
Reopen?

> - Incorrect(?) dependencies: according to docs/DEPENDENCIES
> Requires: pyclutter-cairo should be pycairo
> Requires: python-storm-sqlite should be python-storm
pycairo is called pyclutter-cairo in Fedora, and pyclutter-storm-sqlite pulls in python-storm anywaym but when I set Requires: python-storm, it pulls in the mysql backend, although this package needs the sqlite backend.

> - Unnecessary dependencies
> Requires: pyclutter-gtk
> Requires: pyclutter-gst
> Drop these if you don't have a valid reason to have them.
The program doesn't run without them, so I think I should leave them in :-)

> - Missing Requires: hicolor-icon-theme for dir ownership. Please add also the
> macros to update the icon cache
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
> 
Fixed

> **
> 
> rpmlint output is clean.
> 
> 
> MUST: The package does not yet exist in Fedora. The Review Request is not a
> duplicate. OK
> 
> MUST: The spec file for the package is legible and macros are used
> consistently. OK
> - Please add an empty line (or two as you seem to use) between %build and
> %install.
Fixed.

> MUST: The package must be named according to the Package Naming Guidelines. OK
> - Not a (pure) python module so naming is OK.
> 
> MUST: The spec file name must match the base package %{name}. OK
> MUST: The package must be licensed with a Fedora approved license and meet the 
> Licensing Guidelines. OK
> 
> MUST: The License field in the package spec file must match the actual license.
> OK
> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. OK
> MUST: The package MUST successfully compile and build into binary rpms. OK
> MUST: The spec file MUST handle locales properly. OK
> MUST: Optflags are used and time stamps preserved. OK
> MUST: A package must own all directories that it creates or require the package
> that owns the directory. OK
> 
> 
> MUST: Files only listed once in %files listings. NEEDSWORK
> 
> - For consistency, use
>  %{_bindir}/entertainer*
> instead of
>  %{_bindir}/%{name}*
> (You could also list the four files explicitly.)
Fixed

> - Change
>  %{python_sitelib}/entertainerlib*
> to
>  %{python_sitelib}/entertainerlib/
Fixed

> and
>  %{python_sitelib}/Entertainer-0.4-py2.6.egg-info
> to
>  %{python_sitelib}/Entertainer-*.egg-info
> as you otherwise won't be able to build the spec file in Fedora 10 which has
> Python 2.5.
Fixed

> 
> - Change
>  %{_datadir}/entertainer*
> to
>  %{_datadir}/entertainer/
Fixed

> - Change
>  %{_datadir}/icons/hicolor/*
> to
>  %{_datadir}/icons/hicolor/*/apps/entertainer.png
> (You don't want to own system directories.)
Oops, of course not. Fixed.

> MUST: Debuginfo package is complete. N/A
> MUST: Permissions on files must be set properly. OK
> MUST: Clean section exists. OK
> 
> MUST: Large documentation files must go in a -doc subpackage. N/A
> - developer_documentation.pdf is 237K, which is OK compared to the rest of the
> package.
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. OK
> MUST: Desktop files are installed properly. OK
> MUST: No file conflicts with other packages and no general names. OK
> MUST: Buildroot cleaned before install. OK
> SHOULD: %{?dist} tag is used in release. OK
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. OK
> SHOULD: The package builds in mock. OK  

New SPEC: http://julian.fedorapeople.org/entertainer/entertainer.spec
New SRPM: http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-2.fc11.src.rpm

Comment 3 Julian Aloofi 2009-07-25 15:48:42 UTC
Created attachment 355149 [details]
The mock traceback for the rawhide build

Comment 4 Susi Lehtola 2009-07-25 19:10:34 UTC
The package builds fine in rawhide, koji build at
http://koji.fedoraproject.org/koji/taskinfo?taskID=1514269

I wasn't able to build it locally in mock, though, due to some errors like
 rpmlib(PayloadIsXz) <= 5.2-1 is needed by libstdc++-devel-4.4.1-2.x86_64
 rpmlib(PayloadIsXz) <= 5.2-1 is needed by cpp-4.4.1-2.x86_64
Rawhide is currently in a bit of a state of mess as F12 mass rebuilds are taking place.

**

I think this package should be good to go now. You still have to do the second unofficial review, don't you?


A couple of final, mostly stylistic comments:


- I'd like to see more comments in the spec file. Even though the lines may seem clear to you now, they might not in the future (or for other people).

I suggest dividing the Requires in two parts, e.g.
 # Needed for dir ownership
 Requires: hicolor-icon-theme
 # These packages are needed for operation
 Requires: pyclutter-cairo
and so on. N.B. Remember to especially note the exceptions in the list, such as pyclutter-storm-sqlite instead of plain pyclutter-storm.

- Instead of
 #We have .png and .svg files:
 %{_datadir}/icons/hicolor/*/apps/entertainer.???
you could just write
 %{_datadir}/icons/hicolor/*/apps/entertainer.png
 %{_datadir}/icons/hicolor/*/apps/entertainer.svg
or even
 %{_datadir}/icons/hicolor/*/apps/entertainer.png
 %{_datadir}/icons/hicolor/scalable/apps/entertainer.svg

Comment 5 Julian Aloofi 2009-07-25 20:20:18 UTC
I added some more useful comments in the SPEC file.
SRPM: http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-3.fc11.src.rpm
SPEC: http://julian.fedorapeople.org/entertainer/entertainer.spec


> I think this package should be good to go now. You still have to do the second
> unofficial review, don't you?
The second? If both are valid, I already did two, #508352 and #507943
I can do another one if you like.

Comment 6 Susi Lehtola 2009-07-25 20:26:53 UTC
(In reply to comment #5)
> I added some more useful comments in the SPEC file.
> SRPM:
> http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-3.fc11.src.rpm
> SPEC: http://julian.fedorapeople.org/entertainer/entertainer.spec

Very good.

> > I think this package should be good to go now. You still have to do the second
> > unofficial review, don't you?
> The second? If both are valid, I already did two, #508352 and #507943
> I can do another one if you like.  

Right you are. I must have mistaken you for someone else.


APPROVED

Comment 7 Susi Lehtola 2009-07-25 20:28:52 UTC
Oh, by the way, you'll have to change your bugzilla email to the one your FAS email points to, the package database can't handle discrepancies between BZ and FAS (yet).

Don't worry - emails are only visible to people logged in to bugzilla.

Comment 8 Julian Aloofi 2009-07-25 20:49:37 UTC
Ok, changed my bugzilla mail.
I'll request membership in the packager group now.

Comment 9 Julian Aloofi 2009-07-25 20:51:28 UTC
Requested. I'll have to do the CVS stuff for both packages when I'm member, right?

Comment 10 Susi Lehtola 2009-07-25 21:06:07 UTC
I've sponsored you.

(In reply to comment #9)
> Requested. I'll have to do the CVS stuff for both packages when I'm member,
> right?  

Yes. Once the CVS modules have been created, you can import the packages, build them and submit them as updates to Fedora 10 and 11.

http://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner

Comment 11 Julian Aloofi 2009-07-25 21:18:09 UTC
New Package CVS Request
=======================
Package Name: entertainer
Short Description: A simple media center
Owners: julian
Branches: F-10 F-11
InitialCC:

Comment 12 Rahul Sundaram 2009-07-25 22:32:27 UTC
Few notes:

This pulls in both MySQL and PostgreSQL and their python bindings which I assume is because it can support either databases but you should look into splitting up the packages so that I do need both installed

developer_documentation.pdf needn't be part of a media center for end users, I think. 

In my system, debuginfo package doesn't get generated.

Comment 13 Rahul Sundaram 2009-07-25 22:34:08 UTC
In Fedora 11, I get 

# entertainer
Traceback (most recent call last):
  File "/usr/bin/entertainer", line 12, in <module>
    main()
  File "/usr/lib/python2.6/site-packages/entertainerlib/frontend/__init__.py", line 33, in main
    frontend_client = FrontendClient()
  File "/usr/lib/python2.6/site-packages/entertainerlib/frontend/frontend_client.py", line 40, in __init__
    self.backend_connection = self.initialize_backend_connection()
  File "/usr/lib/python2.6/site-packages/entertainerlib/frontend/frontend_client.py", line 63, in initialize_backend_connection
    backend_connection = BackendConnection()
  File "/usr/lib/python2.6/site-packages/entertainerlib/frontend/backend_connection.py", line 30, in __init__
    self.message_bus_proxy.connectToMessageBus()
  File "/usr/lib/python2.6/site-packages/entertainerlib/backend/core/message_bus_proxy.py", line 63, in connectToMessageBus
    self.socket_to_server.connect(('localhost', self.config.get_port()))
  File "<string>", line 1, in connect
socket.error: [Errno 111] Connection refused

Comment 14 Julian Aloofi 2009-07-25 23:05:04 UTC
(In reply to comment #12)
> Few notes:
> 
> This pulls in both MySQL and PostgreSQL and their python bindings which I
> assume is because it can support either databases but you should look into
> splitting up the packages so that I do need both installed
These get pulled in by python-imdb, I can't do anything. If this is a problem, then it's the problem of the python-imdb maintainer.

> developer_documentation.pdf needn't be part of a media center for end users, I
> think. 
It's not a very big file, should I split it into a doc package anyway?

> In my system, debuginfo package doesn't get generated.  
It's noarch, so we don't have a debuginfo package.


To the socket error, I'll definitely do some research on that. It doesn't occur here, so maybe this is an upstream bug.

Comment 15 Susi Lehtola 2009-07-25 23:50:32 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > Few notes:
> > 
> > This pulls in both MySQL and PostgreSQL and their python bindings which I
> > assume is because it can support either databases but you should look into
> > splitting up the packages so that I do need both installed
> These get pulled in by python-imdb, I can't do anything. If this is a problem,
> then it's the problem of the python-imdb maintainer.
> 
> > developer_documentation.pdf needn't be part of a media center for end users, I
> > think. 
> It's not a very big file, should I split it into a doc package anyway?

The RPM is IIRC 3.1MB (gzip'd), and the pdf file in %doc is just a couple of hundred KB. Splitting a separate -doc is only required for large documentation (compared to the rest of package).

Of course, considering that only a small fraction of users actually want that pdf, branching might be a good idea. Savings of a couple of hundred KB adds up when you have thousands of downloaders.

It's possible, but not required. (In reply to comment #13)
> In Fedora 11, I get 
> 
> # entertainer

Were you really running it as root? That might pose a lot of problems, starting from pulseaudio.

Comment 16 Rahul Sundaram 2009-07-26 00:05:42 UTC
Yes, I think splitting up the doc would be a good idea. I just don't see the value of developer documentation for regular users. It just wastes space. If you don't want to split it, just don't package it at all. Whoever wants it can get it from the website. 

Do you want to file a RFE for python-imdb? Considering it is a dependency for your package, any split there would benefit you as well. Just as a clarificaiton, I am not running it as root and sorry for the confusion.

Comment 17 Julian Aloofi 2009-07-26 10:42:56 UTC
OK, Ill split it up ASAP and ask the python-imdb maintainer if it's even possible to split the package. Maybe it really depends on both.

Comment 18 Julian Aloofi 2009-07-26 11:49:02 UTC
Splitted up, rebuilt and checked in mock for F10 and F11:
SPEC: http://julian.fedorapeople.org/entertainer/entertainer.spec
SRPM: http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-4.fc11.src.rpm

Regarding the socket error: Are you online when running entertainer? There seem to be some issues when not connected to the internet, see upstream bug report:
https://bugs.launchpad.net/entertainer/+bug/243309
Could also be a firewall problem.

Comment 19 Kevin Fenzi 2009-07-26 19:44:06 UTC
Shall we hold off here on cvs until Rahul's concerns are all solved?

Comment 20 Rahul Sundaram 2009-07-26 20:08:49 UTC

Confirmed that the split works however the same problem with the application not even starting. 100% reproducible. I turned off the firewall. Made sure I am online and it makes no difference. 

Not sure you want to hold on before importing it but I request you to test more and report the problem upstream before importing if you can. If I am the only person experiencing it, it wouldn't matter much.

Comment 21 Julian Aloofi 2009-07-26 20:17:17 UTC
I'll set up another system in a virtual machine, using x64, and see whether I can reproduce that error. I'd say we should hold this package as long as we can't say it's exclusively happening to Rahul.

Comment 22 Julian Aloofi 2009-07-26 21:50:58 UTC
I succesfully installed and ran it in a virtual machine (Windows host). The system was unpatched (apart from the dependencies). Worked fine.
Considering that I think we can open a cvs branch, but please wait until I make another release (coming in a couple of hours), I need to include two other .desktop files for the preferences manager and the collection manager, because they're not integrated into the media center. I'll also push them upstream.

Comment 23 Julian Aloofi 2009-07-26 22:40:59 UTC
I updated the package, here are the new files:
SPEC: http://julian.fedorapeople.org/entertainer/entertainer.spec
SRPM: http://julian.fedorapeople.org/entertainer/entertainer-0.4.2-5.fc11.src.rpm

As I can't reproduce Rahuls problems, I guess this should be good to go.
Rahul, if you have the time and want to see it fixed, could you report it upstream yourself? I won't be very useful if upstream asks me to try a patch or something like that.

Comment 24 Kevin Fenzi 2009-07-28 04:46:07 UTC
cvs done.

Comment 25 Fedora Update System 2009-07-28 11:31:31 UTC
entertainer-0.4.2-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/entertainer-0.4.2-5.fc10

Comment 26 Fedora Update System 2009-07-28 11:33:14 UTC
entertainer-0.4.2-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/entertainer-0.4.2-5.fc11

Comment 27 Fedora Update System 2009-07-29 21:28:15 UTC
entertainer-0.4.2-5.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 entertainer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-8077

Comment 28 Fedora Update System 2009-07-29 21:31:58 UTC
entertainer-0.4.2-5.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 entertainer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8099

Comment 29 Fedora Update System 2009-08-15 21:41:56 UTC
entertainer-0.4.2-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2009-08-15 21:43:48 UTC
entertainer-0.4.2-5.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.