Bug 229651 - Review Request: Democracy - A internet TV video player
Review Request: Democracy - A internet TV video player
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-22 09:43 EST by Thorsten Scherf
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-24 05:39:39 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Thorsten Scherf 2007-02-22 09:43:08 EST
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 09:48:18 EST
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 09:59:53 EST
(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 10:05:34 EST
> 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 10:11:26 EST
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 10:25:30 EST
(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 11:39:48 EST
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 11:54:04 EST
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 12:04:28 EST
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 12:19:31 EST
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 12:58:02 EST
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 13:06:17 EST
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 13:15:33 EST
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 13:19:09 EST
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 16:02:13 EST
New Package CVS Request
=======================
Package Name: Democracy
Short Description: Internet TV and video player
Owners: tscherf@redhat.com
Branches: FC-5 FC-6 devel
InitialCC: 
Comment 15 Dennis Gilmore 2007-02-23 08:03:10 EST
branched
Comment 16 Dennis Gilmore 2007-02-23 08:03:29 EST
branched
Comment 17 Michael Schwendt 2007-02-23 17:46:24 EST
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-23 19:06:16 EST
> Is there a good reason...

not really, is it a problem?
Comment 19 Michael Schwendt 2007-02-23 21:35:56 EST
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 05:39:39 EST
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.