Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 201006 - Review Request: HelixPlayer
Summary: Review Request: HelixPlayer
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: FE-ACCEPT 206214
TreeView+ depends on / blocked
 
Reported: 2006-08-02 08:24 UTC by Michael J Knox
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-29 14:40:59 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Michael J Knox 2006-08-02 08:24:01 UTC
Spec URL: http://www.knox.net.nz/~michael/HelixPlayer.spec
SRPM URL: http://www.knox.net.nz/~michael/HelixPlayer-1.0.7-1.src.rpm

Description: 
Helix Player is an open-source media player built in the Helix 
Community for consumers. Built using GTK, it plays open source formats, 
like Ogg Vorbis and Theora using the powerful Helix DNA Client Media 
Engine.

Comment 1 Parag AN(पराग) 2006-08-02 08:30:33 UTC
Finally HelixPlayer arrived in Extras now

Comment 2 Aurelien Bompard 2006-08-11 20:18:40 UTC
Needs work:
* The BuildRoot must be cleaned at the beginning of %install
* It doesn't look like the build uses the $RPM_OPT_FLAGS
* The spec files has a mixed use of spaces and tabs
* Directory /usr/lib/helix/ is unowned
* The desktop file should be installed with desktop-file-install and with the
vendor prefix set to "fedora" (wiki: PackagingGuidelines#desktop)
* Desktop file: the Categories tag should contain X-Fedora
  (wiki: PackagingGuidelines#desktop)
* The translation files are not properly tagged. Use the %find_lang macro
  (wiki: Packaging/ReviewGuidelines)
* Scriptlets: missing "gtk-update-icon-cache" in %post and postun, since you
install icons to %_datadir/icons/hicolor. (wiki: ScriptletSnippets)

Comment 3 Michael J Knox 2006-08-21 00:08:17 UTC
Hey, just a quick ping to let you know that I am alive, just still in the throws
of unpacking/new job/etc etc. I hope to tidy this review up before/by the end of
the week. Thanks for your patience. 

Comment 4 Aurelien Bompard 2006-09-13 10:41:14 UTC
Ping ? Amarok depends on HelixPlayer, so right now it's a broken dependency on
FC6-beta.

Comment 5 Michael J Knox 2006-09-13 18:59:06 UTC
Yes, I am still here. Work commitments are taking there toll at the moment. Will
work on it over the weekend. 

Comment 6 Aurelien Bompard 2006-09-14 15:20:21 UTC
Sure, it I can do anything to help, feel free to ask.

Comment 7 Aurelien Bompard 2006-09-19 14:34:54 UTC
Michael, I've read that you have many other things to do at the moment, so
here's my take at the modifications :
http://gauret.free.fr/fichiers/rpms/fedora/HelixPlayer.spec
And the diffs : http://gauret.free.fr/fichiers/rpms/fedora/HelixPlayer.diff

I did not manage to make the build system obey $RPM_OPT_FLAGS, this build system
is much too complicated for me (and it does not look at the env vars).

I think the rest is fixed (except the mixed-use-of-spaces-and-tabs, it's wrong,
there is only tabs to indent)

Michael, if you like the changes, just use them, and HelixPlayer will be good to
import as far as I'm concerned.

Comment 8 Thorsten Leemhuis 2006-09-22 10:09:20 UTC
(In reply to comment #7)
> Michael, if you like the changes, just use them, and HelixPlayer will be good to
> import as far as I'm concerned.

Wouldn't it be simply better now if you Aurelien submits Helix for review and
someone else reviews it? (maybe even mjk?)


Comment 9 Aurelien Bompard 2006-09-22 10:41:15 UTC
I could do that, but I'm not really interested in maintaining it.
So I'm ok with submitting it as long as Michael swears he'll take it over later
on ;)

Comment 10 Thorsten Leemhuis 2006-09-22 11:19:25 UTC
(In reply to comment #9)
> I could do that, but I'm not really interested in maintaining it.
> So I'm ok with submitting it as long as Michael swears he'll take it over later
> on ;)

Quoting
https://www.redhat.com/archives/fedora-extras-list/2006-September/msg00430.html

"Once Fedora Core/Extras 6 is released, I will put all my packages up on the
orphans wiki page." It IMHO doesn't help much to get a package in FE6 without a
maintainer for the longer term...


Comment 11 Aurelien Bompard 2006-09-22 12:17:45 UTC
> It IMHO doesn't help much to get a package in FE6 without a maintainer for the
> longer term...

My point exactly. Michael also said that he'll be able to contribute again once
he has more time. I'm willing to maintain HelixPlayer for a couple of months (or
half a year, or even for a year if need be) as long as I don't have to maintain
it for all eternity.

OK, so if you guys want to review this :
http://gauret.free.fr/fichiers/rpms/fedora/HelixPlayer-1.0.7-2.src.rpm
It's basically an rpmbuild -bs on the spec file in comment 7

Comment 12 Michael J Knox 2006-09-24 21:55:04 UTC
Hi Aurelien, 

Thanks for working on this! Been a bit busy of late :( I am working on juggling
things so I can remain a contrib dev and not have to orphan up all my packages. 

If you don't mind continuing with the packaging this, we can discuss long term
maintainership off line once I am back on my feet, if you like?



Comment 13 Aurelien Bompard 2006-09-28 15:34:49 UTC
OK, but for now I'd really like someone to review my spec file, in order to fix
the dependency problem of Amarok in FE6

I'll maintain HelixPlayer as long as you haven't enough time for it.

Comment 14 Rex Dieter 2006-09-28 15:42:35 UTC
Aurelien, I'll consider you the submittor, and I'll take over reviewing this, 
if that's ok with you. (:

Comment 15 Rex Dieter 2006-09-28 15:48:53 UTC
A couple oddities stick out to me initially:

1.  Requires: libbluecurve.so%{libdepssuffix} 

Any idea on the rationale for this?  Kinda like the imo crazy dep on 
libbluecurve in openoffice.org?  Can this be dropped?  Maybe I'll just try it 
and see.

2.  Requires: %{_libdir}/mozilla/plugins
I'm torn here, is HelixPlayer *really* dependant on a webbrowser, or is this 
just for directory ownership?  If the latter, imo, this might be better off 
split into a HelixPlayer-plugin subpkg.


Comment 16 Aurelien Bompard 2006-09-28 16:46:11 UTC
Thanks for reviewing Rex !

1. : no idea, it was there in the core package, so I didn't change it. It looks
like it's 64bits-specific. If you have access to such a machine, could you test ?

2. : right, I splitted off the plugin :

http://gauret.free.fr/fichiers/rpms/fedora/HelixPlayer-1.0.7-3.src.rpm



Comment 17 Rex Dieter 2006-09-28 18:14:09 UTC
> It looks like it's 64bits-specific. If you have access to such a machine, 
> could you test ?

maybe not, see:
ExcludeArch:    ppc64 x86_64 s390 s390x ia64


Comment 18 Rex Dieter 2006-09-28 18:15:01 UTC
ok, reference bug #135709

Comment 19 Rex Dieter 2006-09-28 18:17:12 UTC
I don't think I agree with the hack, I say omit it.

Comment 20 Rex Dieter 2006-09-28 18:37:54 UTC
Suggestions:

1.  Make buildable everywhere
-BuildRequires: libogg-devel, libXt-devel, libXv-devel
+BuildRequires: libogg-devel
 BuildRequires: desktop-file-utils
+%if "%{?fedora}" > "4" || "%{?rhel}" > "4"
+BuildRequires: libXt-devel libXv-devel
+%else
+BuildRequires: xorg-x11-devel
+%endif

2.  Drop
Requires: libbluecurve
hack

3.  -plugin should
Requires:   %{name} = %{version}-%{release}

4.  drop deprecated use-of/references-to %_datadir/pixmaps

5.  in %install, recommend using the 'install -p' command in place of 'cp', be
careful in most places to use -m644, but for bins/libs use -m755

Address these, and I'll approve this.


Comment 21 Aurelien Bompard 2006-09-28 21:47:38 UTC
OK, fixed :
http://gauret.free.fr/fichiers/rpms/fedora/HelixPlayer-1.0.7-4.src.rpm

1. : I've added ' || "%{?fedora}%{?rhel}" == "" ' to have rpm pick the Right Dep
if neither %fedora not %rhel is defined (manual rebuild)

2. : agreed

3. : actually, it's Requires:   %{name} = %{epoch}:%{version}-%{release}
     Took me 10 minutes to figure it out. I hate epochs.

4. and 5. : done.

Comment 22 Rex Dieter 2006-09-29 12:50:09 UTC
OK, looks good, APPROVED.

Comment 23 Aurelien Bompard 2006-09-29 14:40:59 UTC
Imported and built, thanks a lot !


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