Bug 201006 - Review Request: HelixPlayer
Review Request: HelixPlayer
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: FE-ACCEPT 206214
  Show dependency treegraph
 
Reported: 2006-08-02 04:24 EDT by Michael J Knox
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-29 10:40:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Michael J Knox 2006-08-02 04:24:01 EDT
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 04:30:33 EDT
Finally HelixPlayer arrived in Extras now
Comment 2 Aurelien Bompard 2006-08-11 16:18:40 EDT
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-20 20:08:17 EDT
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 06:41:14 EDT
Ping ? Amarok depends on HelixPlayer, so right now it's a broken dependency on
FC6-beta.
Comment 5 Michael J Knox 2006-09-13 14:59:06 EDT
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 11:20:21 EDT
Sure, it I can do anything to help, feel free to ask.
Comment 7 Aurelien Bompard 2006-09-19 10:34:54 EDT
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 06:09:20 EDT
(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 06:41:15 EDT
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 07:19:25 EDT
(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 08:17:45 EDT
> 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 17:55:04 EDT
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 11:34:49 EDT
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 11:42:35 EDT
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 11:48:53 EDT
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 12:46:11 EDT
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 14:14:09 EDT
> 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 14:15:01 EDT
ok, reference bug #135709
Comment 19 Rex Dieter 2006-09-28 14:17:12 EDT
I don't think I agree with the hack, I say omit it.
Comment 20 Rex Dieter 2006-09-28 14:37:54 EDT
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 17:47:38 EDT
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 08:50:09 EDT
OK, looks good, APPROVED.
Comment 23 Aurelien Bompard 2006-09-29 10:40:59 EDT
Imported and built, thanks a lot !

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