Finally HelixPlayer arrived in Extras now

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)

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. 

Ping ? Amarok depends on HelixPlayer, so right now it's a broken dependency on
FC6-beta.

Yes, I am still here. Work commitments are taking there toll at the moment. Will
work on it over the weekend. 

Sure, it I can do anything to help, feel free to ask.

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.

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

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 ;)

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

> 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

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?

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.

Aurelien, I'll consider you the submittor, and I'll take over reviewing this, 
if that's ok with you. (:

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.

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

> 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

ok, reference bug #135709

I don't think I agree with the hack, I say omit it.

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.

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.

OK, looks good, APPROVED.

Imported and built, thanks a lot !