Bug 222326
Summary: | Review Request: gxine - GTK frontend for the xine multimedia library | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Martin Sourada <martin.sourada> | ||||||||
Component: | Package Review | Assignee: | Michel Alexandre Salim <michel.salim> | ||||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||||
Severity: | medium | Docs Contact: | |||||||||
Priority: | medium | ||||||||||
Version: | rawhide | CC: | michel.salim, mola.mp, mtasaka, stefan.hoelldampf | ||||||||
Target Milestone: | --- | Flags: | michel.salim:
fedora-review+
kevin: fedora-cvs+ |
||||||||
Target Release: | --- | ||||||||||
Hardware: | All | ||||||||||
OS: | Linux | ||||||||||
Whiteboard: | |||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||
Doc Text: | Story Points: | --- | |||||||||
Clone Of: | Environment: | ||||||||||
Last Closed: | 2007-03-26 19:19:52 UTC | Type: | --- | ||||||||
Regression: | --- | Mount Type: | --- | ||||||||
Documentation: | --- | CRM: | |||||||||
Verified Versions: | Category: | --- | |||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||
Embargoed: | |||||||||||
Attachments: |
|
Description
Martin Sourada
2007-01-11 17:58:37 UTC
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. ;) |