Bug 222326

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

*** Bug 213511 has been marked as a duplicate of this bug. ***

(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.

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

(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.

the current SRPM builds successfully in mock for Fedora Development/i386 and
rpmlint is silent on all rpms

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

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

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". 

(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

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.

(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).

/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

(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.

(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").

(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?

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

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

(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

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 on attachment 146444 [details]
stream end crash patch

Fixed in new upstream release.

New upstream 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-1.src.rpm

I hope I can check this package by tomorrow...

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

(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

I got sponsored with another package. Removed FE-NEEDSPOSOR blocker.

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.

Ugh, some problems with hosting, but I hopefully solved it so the files are
accessible again.

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

(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...

New Package CVS Request
=======================
Package Name: gxine
Short Description: GTK multimedia player using xine-lib engine
Owners: martin.sourada
Branches: FC-6
InitialCC: 

Branched

mockbuild of 0.5.11-3 says:
------------------------------------
checking for xscreensaver-command... no
------------------------------------
Perhaps this can be enabled by adding "xscreensaver-base" to
BuildRequires.

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??

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?

(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.

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.

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.

(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.

Package Change Request
======================
Package Name: gxine
New Branches: EL-4 EL-5

cvs done.

Package Change Request
======================
Package Name: gxine
New Branches: F-9

mass branching has been completed, so this package should now have a F-9 branch. ;)