Bug 229651 - Review Request: Democracy - A internet TV video player
Summary: Review Request: Democracy - A internet TV video player
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-02-22 14:43 UTC by Thorsten Scherf
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-24 10:39:39 UTC
Type: ---
Embargoed:
rdieter: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Thorsten Scherf 2007-02-22 14:43:08 UTC
Spec URL: http://people.redhat.com/tscherf/rpms/fedora/Democracy.spec
SRPM URL: http://people.redhat.com/tscherf/rpms/fedora/Democracy-0.9.5-1.src.rpm
Description: Democracy player is a free application that turns your computer into an internet TV video player.

Comment 1 Rex Dieter 2007-02-22 14:48:18 UTC
Awesome!

A couple quick comments:
1. No need to %ghost .pyo files anymore, as a matter of fact, it is 
discouraged:
http://fedoraproject.org/wiki/Packaging/Python

2.  Use %find_lang instead of manually including locale bits:
%{_datadir}/locale/*/LC_MESSAGES/democracyplayer.mo

3.  You include
%post
update-desktop-database %{_datadir}/applications
but not for %postun ?

Comment 2 Gianluca Sforna 2007-02-22 14:59:53 UTC
(In reply to comment 
> A couple quick comments:
> 1. No need to %ghost .pyo files anymore, as a matter of fact, it is 
> discouraged:
> http://fedoraproject.org/wiki/Packaging/Python

moreover, I think the python-abi require line could go away.

> 3.  You include
> %post
> update-desktop-database %{_datadir}/applications
> but not for %postun ?

and those lines should probably end with "&> /dev/null ||:" as specified in:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets


Moreover, do they fixed the x86_64 issues present in earlier versions?



Comment 3 Rex Dieter 2007-02-22 15:05:34 UTC
> Moreover, do they fixed the x86_64 issues present in earlier versions?

"they" being who?  democracy devs?
"the x86_64 issues" being what exactly?  didn't build?  didn't run properly?

Comment 4 Xavier Lamien 2007-02-22 15:11:26 UTC
please, use %{_mandir}/man1/ instead of %{_datadir}/man/man1/

> # Include files and dirs below %{python_sitelib} (for noarch packages) 

Where ?

the use of cd platform/gtk-x11 looks good.
personnaly, i prefer to use pushd-command to enter in some directory and
popd-command to exit this one (same thing for %build section).
such as:
----
pushd gtk-x11
python setup.py install -O1 --skip-build --root $RPM_BUILD_ROOT
popd
----
i think it's better for a clean review.

Comment 5 Gianluca Sforna 2007-02-22 15:25:30 UTC
(In reply to comment #3)
> > Moreover, do they fixed the x86_64 issues present in earlier versions?
> 
> "they" being who?  democracy devs?
> "the x86_64 issues" being what exactly?  didn't build?  didn't run properly?

sorry for the out-of-context comment.
I was referring to this:
https://www.redhat.com/archives/fedora-extras-list/2006-November/msg00104.html


Comment 6 Thorsten Scherf 2007-02-22 16:39:48 UTC
1) removed %ghost from files section
2) used find_lang 
3) created a postun 
4) added "&> /dev/null ||:" to the Scriptlet
5) AFAIK the x86_64 bug should be fixed, can't test it myself, maybe someone
else can verify this

http://people.redhat.com/tscherf/rpms/fedora/Democracy.spec
http://people.redhat.com/tscherf/rpms/fedora/Democracy-0.9.5-2.src.rpm


Comment 7 Rex Dieter 2007-02-22 16:54:04 UTC
Looking good, I can review this.

1. SHOULD simplify things and just include in %files:
%{python_sitearch}/democracy/
instead of all the %dirs and * globbing.

2.  Requires: firefox
Other apps that build against firefox-devel need/use a *versioned* requires 
here, in effect Require'ing the same version of firefox they were built 
against.  Is that the case here?  (or maybe not worry about it (: )

3.  Requires: python-abi ...
shouldn't be explictly required (when using python >= 2.4 anyway).

4.  Requires:	xine-lib gnome-python2-gtkmozembed libfame gnome-python2-gconf 
dbus-python
Are *all* of these really explicitly required?  In particular, xine-lib 
libfame should get auto'req'd by rpm (if not, it's ok to keep the Requires).

Comment 8 Thorsten Scherf 2007-02-22 17:04:28 UTC
1) simplified the files section
3) removed python-abi req
4) rpm should take care about this, true, removed the req

about 2) not sure if we need a versioned firefox requirement. I tested this
package on 2 difeerent machines with different firefox versions without
problems, so it should work. could you verify this?

http://people.redhat.com/tscherf/rpms/fedora/Democracy.spec
http://people.redhat.com/tscherf/rpms/fedora/Democracy-0.9.5-3.src.rpm


Comment 9 Rex Dieter 2007-02-22 17:19:31 UTC
mock build failed on Democracy-0.9.5-3.src.rpm
...
Package config error:
pkg-config --list-all outputted the following error:
Package xfixes was not found in the pkg-config search path.
Perhaps you should add the directory containing `xfixes.pc'
to the PKG_CONFIG_PATH environment variable
Package 'xfixes', required by 'Xcursor', not found

error: Bad exit status from /var/tmp/rpm-tmp.89920 (%build)

Either a missing BR or an X-dep missing/bug.

Comment 10 Rex Dieter 2007-02-22 17:58:02 UTC
after adding 
BuildRequires: libXcursor-devel libXfixes-devel
now I see
RuntimeError: pkg-config --cflags --libs gtk+-2.0 glib-2.0 pygtk-2.0
firefox-gtkmozembed firefox-xpcom outputted the following error:
Package gtk+-2.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `gtk+-2.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'gtk+-2.0' found

adding 
BuildRequires: gtk2-devel
iterating again...  finished build.

So, looks like we need to add:
BuildRequires: libXcursor-devel libXfixes-devel
BuildRequires: gtk2-devel

Comment 11 Rex Dieter 2007-02-22 18:06:17 UTC
These look mostly harmless:

$ rpmlint Democracy-0.9.5-3.fc7.i386.rpm
E: Democracy non-executable-script
/usr/lib/python2.5/site-packages/democracy/coverage.py 0644
E: Democracy non-executable-script
/usr/lib/python2.5/site-packages/democracy/timetemplates.py 0644
W: Democracy wrong-file-end-of-line-encoding /usr/share/doc/Democracy-0.9.5/CREDITS
E: Democracy non-executable-script
/usr/lib/python2.5/site-packages/democracy/feedparser.py 0644

Add the missing BR's and looks like we have a winner.

Comment 12 Thorsten Scherf 2007-02-22 18:15:33 UTC
Looks like we figured out the same thing during the same time. :) Was also
looking  for addional BR after Mock building failed. Added the necessary BR and
now it builds in Mock without problems.

http://people.redhat.com/tscherf/rpms/fedora/Democracy.spec
http://people.redhat.com/tscherf/rpms/fedora/Democracy-0.9.5-4.src.rpm

Comment 13 Rex Dieter 2007-02-22 18:19:09 UTC
Democracy rules, APPROVED.

Be careful about removing some of the python runtime Req's, I didn't verify if 
those are still Required or not:  gnome-python2-gtkmozembed 
gnome-python2-gconf dbus-python

Comment 14 Thorsten Scherf 2007-02-22 21:02:13 UTC
New Package CVS Request
=======================
Package Name: Democracy
Short Description: Internet TV and video player
Owners: tscherf
Branches: FC-5 FC-6 devel
InitialCC: 

Comment 15 Dennis Gilmore 2007-02-23 13:03:10 UTC
branched

Comment 16 Dennis Gilmore 2007-02-23 13:03:29 UTC
branched

Comment 17 Michael Schwendt 2007-02-23 22:46:24 UTC
Is there a good reason why the spec does unusual manual installation
of documentation via %_defaultdocdir instead of simply doing

%doc README license.txt CREDITS

in the %files section?


Comment 18 Rex Dieter 2007-02-24 00:06:16 UTC
> Is there a good reason...

not really, is it a problem?

Comment 19 Michael Schwendt 2007-02-24 02:35:56 UTC
It makes installation of additional files via short %doc impossible,
because both techniques conflict with eachother. Using the %doc macro
to include files located in the extracted tarball in $RPM_BUILD_DIR
deletes %_defaultdocdir.


Comment 20 Thorsten Scherf 2007-02-24 10:39:39 UTC
I corrected the doc installation style. updated package (0.9.5.1-3) is now in
devel and fc6 branch available. 

will close here.
 


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