Bug 222326 - Review Request: gxine - GTK frontend for the xine multimedia library
Review Request: gxine - GTK frontend for the xine multimedia library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Package Reviews List
:
: 213511 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-11 12:58 EST by Martin Sourada
Modified: 2008-04-22 11:48 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-26 15:19:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
michel.salim: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
stream end crash patch (450 bytes, text/x-patch)
2007-01-24 14:48 EST, Martin Sourada
no flags Details
Patch to keep track of window state, backported from 0.6.0 branch (2.07 KB, patch)
2007-03-13 21:01 EDT, Michel Alexandre Salim
no flags Details | Diff
gdb log of gxine-0.5.11-3 on FC-devel i386 (7.17 KB, text/plain)
2007-03-26 14:48 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Martin Sourada 2007-01-11 12:58:37 EST
Spec URL: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
SRPM URL: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.10-1.src.rpm
Description: 
gxine is a fully-featured free audio/video player for unix-like systems which
uses libxine for audio/video decoding and playback. For more informations on
what formats are supported, please refer to the libxine documentation.
gxine is a gtk based gui for this xine-library alternate to xine-ui.

This is my first package so I need a sponsor.
Comment 1 Dan Horák 2007-01-11 13:58:50 EST
small notes
  - today´s entry in the changelog still refers to version 0.5.9
  - rpmlint gives "W: gxine mixed-use-of-spaces-and-tabs (spaces: line 1, tab:
line 25)" for the spec file, so convert the indentation to only one of them
Comment 2 Mamoru TASAKA 2007-01-11 14:13:40 EST
*** Bug 213511 has been marked as a duplicate of this bug. ***
Comment 3 Martin Sourada 2007-01-11 14:23:37 EST
(In reply to comment #1)
> small notes
>   - today´s entry in the changelog still refers to version 0.5.9
>   - rpmlint gives "W: gxine mixed-use-of-spaces-and-tabs (spaces: line 1, tab:
> line 25)" for the spec file, so convert the indentation to only one of them

Thanks, fixed. I replaced the old versions with the fixed ones.
Comment 4 Josef Bacik 2007-01-11 14:25:34 EST
==== REVIEW CHECKLIST ====
- rpmlint output is clean
- package meets package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- package licensed with open source compatible license
- license matches actual license
- license file included in %doc
- spec written in American english
- spec file is legible
- sources match upstream (tho as a side note the URL didn't work)
0df37a7a38ea0d63129d2590971caabb  /home/josef/gxine-0.5.10.tar.bz2
- package successfully compiles and builds on x86_64 FC6
- all build dependencies listed in BR
- locales handled properly
- no shared libraries
- package is not relocatable
- package owns all directories it creates
- directories it does not create owned by default packages
- no duplicates in %files
- file permissions set properly
- contains proper %clean section
- macro usage consistent
- package contains code
- no large documentation
- files in %doc do not affect runtime
- no header files or static libraries
- no pkgconfig files
- no library files with suffix
- no need for devel subpackage
- no .la files
* desktop file is not installed via the appropriate methods
- package does not own files or directories owned by other packages

====Things that need to be fixed====
* I found the upstream sources, but the URL that was in your spec file did not 
resolve, you may want to find a more permament URL.
* You need to install the desktop file appropriatly, look here for an example
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
  gxine seems to install it by itself, so you will want to do something like 
what I did for geany
	http://www.toxicpanda.com/geany.spec
* for gxine-mozplugin, it would not install because you have

  Requires: %{name}=%{version}-%{release}

  you need to put a space in between the '=', so it looks like this

  Requires: %{name} = %{version}-%{release}

  and then it will work.


I am not sponsored, this is just a review to help me get sponsored.
Comment 5 Martin Sourada 2007-01-11 14:58:18 EST
(In reply to comment #4)
> ====Things that need to be fixed====
> * I found the upstream sources, but the URL that was in your spec file did not 
> resolve, you may want to find a more permament URL.
> * You need to install the desktop file appropriatly, look here for an example
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
>   gxine seems to install it by itself, so you will want to do something like 
> what I did for geany
> 	http://www.toxicpanda.com/geany.spec
> * for gxine-mozplugin, it would not install because you have
> 
>   Requires: %{name}=%{version}-%{release}
> 
>   you need to put a space in between the '=', so it looks like this
> 
>   Requires: %{name} = %{version}-%{release}
> 
>   and then it will work.

Fixed.

New SPEC: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
New SRPM: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.10-2.src.rpm

Thanks for pre-review.
Comment 6 Dan Horák 2007-01-13 07:30:47 EST
the current SRPM builds successfully in mock for Fedora Development/i386 and
rpmlint is silent on all rpms
Comment 7 Michel Alexandre Salim 2007-01-14 02:38:15 EST
Another pre-review (I do not have sponsoring privileges)

- Source tarball downloadable and matches upstream
- Builds fine under mock for fedora-6-x86_64
- Works fine (note: logo won't appear unless xine-lib-extras is installed. Would
be nice to have this once Fedora's RPM supports the 'Suggest' tag)

Note to intended sponsor: this is the continuation of the packaging process
Martin and I did in bz #213511
Comment 8 Michel Alexandre Salim 2007-01-16 18:47:31 EST
Martin, you might want to announce this review in fedora-extras-list, and
mention that it's been pre-reviewed? Developers might not notice that you need
sponsoring otherwise
Comment 9 Ralf Corsepius 2007-01-17 10:31:44 EST
I am still seeing a couple of spots I don't feel comfortable about:

1. The BR: gettext-devel seems superfluous to me.
AFAIS, "BR: gettext" should suffice.


2. The mozplugin has depencency problems wrt. the %{libdir}/mozilla/plugins
directory

It must either
* own %{libdir}/mozilla/plugins
* Require a package providing %{libdir}/mozilla/plugins
* "Requires: %{libdir}/mozilla/plugins"

AFAICT, their is no policy on handling mozilla-plugins, yet, so the most simple
work-around would be to "Requires: firefox". 
Comment 10 Martin Sourada 2007-01-17 13:43:17 EST
(In reply to comment #9)
> I am still seeing a couple of spots I don't feel comfortable about:
> 
> 1. The BR: gettext-devel seems superfluous to me.
> AFAIS, "BR: gettext" should suffice.
Oh, I think you are right. I don't remember, why I put there the gettext-devel
BR... Fixed.

> 2. The mozplugin has depencency problems wrt. the %{libdir}/mozilla/plugins
> directory
> 
> It must either
> * own %{libdir}/mozilla/plugins
> * Require a package providing %{libdir}/mozilla/plugins
> * "Requires: %{libdir}/mozilla/plugins"
> 
> AFAICT, their is no policy on handling mozilla-plugins, yet, so the most simple
> work-around would be to "Requires: firefox". 
> 

Well, I didn't realize that. I looked at the totem package (which has also
mozplugin subpackage) and noticed that it is also not handled there, so I did
not think it necessary. However, I followed your advice and added Requires:
firefox. Thanks for the tip. :-)

New SPEC: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
New SRPM: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.10-3.src.rpm
Comment 11 Ville Skyttä 2007-01-17 17:46:13 EST
While it has its drawbacks in the form of yum having to download filelists XML,
the least intrusive way I'm aware of to currently require "a browser which uses
Mozilla plugins and loads them from %{_libdir}/mozilla/plugins" (I'm not sure if
that's the goal here) is to use "Requires: %{_libdir}/mozilla/plugins".

See eg. bug 131247, bug 207613, and gnash, HelixPlayer, and opensc packages.
Comment 12 Martin Sourada 2007-01-17 18:09:38 EST
(In reply to comment #11)
> While it has its drawbacks in the form of yum having to download filelists XML,
> the least intrusive way I'm aware of to currently require "a browser which uses
> Mozilla plugins and loads them from %{_libdir}/mozilla/plugins" (I'm not sure if
> that's the goal here) is to use "Requires: %{_libdir}/mozilla/plugins".
> 
> See eg. bug 131247, bug 207613, and gnash, HelixPlayer, and opensc packages.

Well, I have not made up my mind fully about that. Temporarily I added
"Requires: firefox", as Ralf suggested, but neither his nor yours solution I see
as ideal. Now I prefer the first one, assuming, that most fedora users have
firefox installed and thus it will work for most people as intended. If I used
"Requires: %{_libdir}/mozilla/plugins" it would solve one problem, while other
would arise. If a user has not got installed a package which owns this
directory, which one yum picks up? I don't suppose it'll ask you which one do
you want. I think the best solution would be, as suggested in bz #131247, a
standalone package which would own these directories and would be required by
all packages that write to or read from these directories (e.g. firefox and
totem-mozplugin).
Comment 13 Thorsten Leemhuis 2007-01-18 03:50:21 EST
/me votes for "%{_libdir}/mozilla/plugins" as that scheme is used by other
packages already and makes it possible to create a spin with gxine, but without
firefox
Comment 14 Martin Sourada 2007-01-18 04:18:03 EST
(In reply to comment #13)
> /me votes for "%{_libdir}/mozilla/plugins" as that scheme is used by other
> packages already and makes it possible to create a spin with gxine, but without
> firefox

Well, I will think about it. But about the spin you talk about - you can have
gxine without firefox installed. The "Require: firefox" is in the -mozplugin
subpackage.
Comment 15 Ralf Corsepius 2007-01-18 04:37:57 EST
(In reply to comment #14)
> (In reply to comment #13)
> > /me votes for "%{_libdir}/mozilla/plugins" as that scheme is used by other
> > packages already 
This doesn't work do to bugs in rpm (Ville: Does this still apply, or this rpm
finally been fixed).

> Well, I will think about it. But about the spin you talk about - you can have
> gxine without firefox installed. The "Require: firefox" is in the -mozplugin
> subpackage.
The problem is not firefox, the problem is removing "%{_libdir}/mozilla/plugins"
the directory, when it's not needed anymore.

That is what I wrote a couple of days ago in another package's review:

- If several packages share a common directory, and if they depend on each other
in a strict hierarchy, then letting the "root package" own this dir is sufficient.
- If they don't depend on each other in a strict hierarchy, all of the packages
must own this directory.

The former doesn't apply to gxine-mozplug => Only the latter is applicable.

"Requires: firefox" would be a hack to force the former approach, though this,
strictly speaking is incorrect and too strict (It's the same "hack" why packages
providing *.pc must "Require: pkgconfig" and packages providing aclocal/*.m4
must "Require: automake". Both these "Require"-ments are hacks/band-aids and
actually wrong, but provided rpm persists to be unfixed/unmaintained, these are
"easy hacks").


Comment 16 Martin Sourada 2007-01-18 05:11:36 EST
(In reply to comment #15)
> The problem is not firefox, the problem is removing "%{_libdir}/mozilla/plugins"
> the directory, when it's not needed anymore.
> 
> That is what I wrote a couple of days ago in another package's review:
> 
> - If several packages share a common directory, and if they depend on each other
> in a strict hierarchy, then letting the "root package" own this dir is sufficient.
> - If they don't depend on each other in a strict hierarchy, all of the packages
> must own this directory.
> 
> The former doesn't apply to gxine-mozplug => Only the latter is applicable.
> 
> "Requires: firefox" would be a hack to force the former approach, though this,
> strictly speaking is incorrect and too strict (It's the same "hack" why packages
> providing *.pc must "Require: pkgconfig" and packages providing aclocal/*.m4
> must "Require: automake". Both these "Require"-ments are hacks/band-aids and
> actually wrong, but provided rpm persists to be unfixed/unmaintained, these are
> "easy hacks").
> 

So what do you suggest then? If the "Requires: firefox" is incorrect hack and
"Requires: %{_libdir}/mozilla/plugins" won't work due to some bug in rpm? Or did
I misunderstand your post?
Comment 17 Martin Sourada 2007-01-18 05:44:23 EST
Ok, so I looked into all packages I am aware of that need
%{_libdir}/mozilla/plugins and except for totem-mozplugin all has "Requires:
%{_libdir}/mozilla/plugins". So I decided to follow the common way of requiring
it and changed my package accordingly.

New SPEC: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
New SRPM: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.10-4.src.rpm
Comment 18 Michel Alexandre Salim 2007-01-24 09:45:34 EST
Could the following patch from gxine's Mercurial tree be applied? It concerns
the application crashing if a modal dialog is open when the current stream ends.

https://sourceforge.net/tracker/?func=detail&atid=109655&aid=1643093&group_id=9655
http://zap.tartarus.org/~ds/hg/gxine/?cmd=changeset;node=1827
Comment 19 Martin Sourada 2007-01-24 14:45:00 EST
(In reply to comment #18)
> Could the following patch from gxine's Mercurial tree be applied? It concerns
> the application crashing if a modal dialog is open when the current stream ends.
> 
> https://sourceforge.net/tracker/?func=detail&atid=109655&aid=1643093&group_id=9655
> http://zap.tartarus.org/~ds/hg/gxine/?cmd=changeset;node=1827

Done.

New SPEC: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
New SRPM: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.10-5.src.rpm
Comment 20 Martin Sourada 2007-01-24 14:48:30 EST
Created attachment 146444 [details]
stream end crash patch

Upstream patch.
Fixes a crash which occurs when some dialog is opened and stream ends. Upstream
bz #1643093
Comment 21 Martin Sourada 2007-02-01 15:00:01 EST
Comment on attachment 146444 [details]
stream end crash patch

Fixed in new upstream release.
Comment 23 Mamoru TASAKA 2007-03-09 02:27:41 EST
I hope I can check this package by tomorrow...
Comment 24 Michel Alexandre Salim 2007-03-13 21:01:40 EDT
Created attachment 150011 [details]
Patch to keep track of window state, backported from 0.6.0 branch

gxine <= 0.5.11 does not keep track of window state (try setting "always on
top", going full screen and then back). Upstream has now fixed this but it's
not going to be released until 0.6.0; meantime, here's the backport of the
patch.

Upstream bug report:
https://sourceforge.net/tracker/?func=detail&atid=359655&aid=1679490&group_id=9655
Comment 25 Martin Sourada 2007-03-14 12:43:13 EDT
(In reply to comment #24)
> Created an attachment (id=150011) [edit]
> Patch to keep track of window state, backported from 0.6.0 branch
> 
> gxine <= 0.5.11 does not keep track of window state (try setting "always on
> top", going full screen and then back). Upstream has now fixed this but it's
> not going to be released until 0.6.0; meantime, here's the backport of the
> patch.
> 
> Upstream bug report:
> https://sourceforge.net/tracker/?func=detail&atid=359655&aid=1679490&group_id=9655

Thanks, included it in new pkg release.

New SPEC: http://feannatar.hostuju.cz/fedora/files/FC6/SPECS/gxine.spec
New SRPM: http://feannatar.hostuju.cz/fedora/files/FC6/SRPMS/gxine-0.5.11-2.src.rpm
Comment 26 Martin Sourada 2007-03-14 18:55:11 EDT
I got sponsored with another package. Removed FE-NEEDSPOSOR blocker.
Comment 27 Michel Alexandre Salim 2007-03-19 22:30:56 EDT
I can't get to the files - are you over your bandwith quota? In which case, if
you e-mail the files to me in a tarball, I'll put it up on my web hosting account.
Comment 28 Martin Sourada 2007-03-21 05:28:14 EDT
Ugh, some problems with hosting, but I hopefully solved it so the files are
accessible again.
Comment 29 Michel Alexandre Salim 2007-03-25 13:34:22 EDT
Approving the package. There's only one small niggle with the spec (see rpmlint
below), please fix before committing to CVS.

• rpmlint (source):
	W: gxine mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 13)

		use Emacs M-x untabify?
• rpmlint (binary x86_64): clean
• package name OK
• spec name OK
• Packaging meets guideline
• License matches
• License text in %doc
• Spec language OK
• Source matches upstream
• Package builds OK (x86_64)
• Build dependencies OK (builds fine in mock)
• Locales handled OK
• Directory ownership OK
• No duplicate files
Comment 30 Martin Sourada 2007-03-25 15:13:08 EDT
(In reply to comment #29)
> Approving the package. There's only one small niggle with the spec (see rpmlint
> • rpmlint (source):
> 	W: gxine mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 13)
> 
> 		use Emacs M-x untabify?

Fixed. I wonder how long will it take to me to learn to use spaces instead of
tabs...
Comment 31 Martin Sourada 2007-03-25 15:20:13 EDT
New Package CVS Request
=======================
Package Name: gxine
Short Description: GTK multimedia player using xine-lib engine
Owners: martin.sourada@seznam.cz
Branches: FC-6
InitialCC: 
Comment 32 Dennis Gilmore 2007-03-25 16:00:40 EDT
Branched
Comment 33 Mamoru TASAKA 2007-03-26 14:26:19 EDT
mockbuild of 0.5.11-3 says:
------------------------------------
checking for xscreensaver-command... no
------------------------------------
Perhaps this can be enabled by adding "xscreensaver-base" to
BuildRequires.
Comment 34 Mamoru TASAKA 2007-03-26 14:48:17 EDT
Created attachment 150929 [details]
gdb log of gxine-0.5.11-3 on FC-devel i386

Well, for me gxine causes segv..
ffplay, xine seems to have no problem (though
both of them are from livna)

Would you know the reason??
Comment 35 Martin Sourada 2007-03-26 15:19:52 EDT
Successfully built, Closing Review with NEXTRELEASE.

In reply to Comment #33
Thanks, I will add it to next with (along with building FC-6 version).

In reply to Comment #34
No, I do not know. Does it happen only to devel version?
Comment 36 Mamoru TASAKA 2007-03-27 05:25:50 EDT
(In reply to comment #35)

> In reply to Comment #34
> No, I do not know. Does it happen only to devel version?

Well, I don't have any machine other than FC-devel i386
and cannot check on other branches.
Comment 37 Michel Alexandre Salim 2007-03-27 18:57:13 EDT
On Emacs, Tab defaults to inserting spaces, which is nice -- it's probably an
option for most other editors too.

Running FC-devel/x86_64 here and didn't see any segfaults. It would sometimes
run slowly and with a greenish tint, but I blame that on my ATi video card that
does not support DRI (and thus the RENDER extension) -- once that happens
repeatedly restarting gxine might crash X, and the safe option is to log out and
log back in (restarting X in the process)

By the way, xscreensaver should *not* be a dependency, I think. Let configure
fail to find it, or explicitly disable it -- the default GNOME screensaver is
gnome-screensaver, which gxine supports just fine.
Comment 38 Michel Alexandre Salim 2007-03-27 18:58:36 EDT
On the other hand, since there's no runtime dependency, I guess it does not make
much of a difference. What's the default screensaver program in FC-6? I can't
recall.
Comment 39 Mamoru TASAKA 2007-03-28 02:37:32 EDT
(In reply to comment #37)
> By the way, xscreensaver should *not* be a dependency, I think. Let configure
> fail to find it, or explicitly disable it -- the default GNOME screensaver is
> gnome-screensaver, which gxine supports just fine.

No, the policy is "support all option as much as possible"
unless you have a _special_ reason why you don't want to support
a certain option. What is the default does not make sense.
Comment 40 Martin Sourada 2007-12-10 07:29:12 EST
Package Change Request
======================
Package Name: gxine
New Branches: EL-4 EL-5
Comment 41 Kevin Fenzi 2007-12-10 11:46:06 EST
cvs done.
Comment 42 Martin Sourada 2008-04-19 10:10:12 EDT
Package Change Request
======================
Package Name: gxine
New Branches: F-9
Comment 43 Kevin Fenzi 2008-04-22 11:48:20 EDT
mass branching has been completed, so this package should now have a F-9 branch. ;) 

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