Bug 452749 (ocp)

This is fantastic. Cubic Player brings a lot of memories. ;)

I'll review this and sponsor you, but in the meantime, please do a couple of
reviews to show your understanding of the packaging guidelines. I may even have
one of my own packages to throw at you.

What is your FAS user name?

My FAS user name is cra.  I'd be glad to look at some of your packages.  Thanks.

Partial review::

ocp-0.1.15-alsa.patch:
-       if ((err=snd_pcm_hw_params_any(alsa_pcm, hwparams)))
+       err=snd_pcm_hw_params_any(alsa_pcm, hwparams);
+        if (err < 0)

This can be done in one line:
+       if ((err=snd_pcm_hw_params_any(alsa_pcm, hwparams)) < 0)

Have the patches been sent upstream?

ocp.spec:
Release: 1.2%{dist}

Please make it just 1%{dist}. We'll start the review at release 1 and increase
that number with each revision until the package is approved.

BuildRequires: libogg-devel

Redundant, required by libvorbis-devel

# ocp contains non-64-bit clean i386 assembly
ExclusiveArch: i386

Not necessary. It builds and works (mostly) on x86_64 (tested!). I've talked to
one of the developers and he said he's working on fixing it on ppc. He also said
it works on sparc.

Also, please put all BuildRequires in separate lines and sort them
alphabetically. It makes them easier to manage and makes cvs diffs smaller and
more readable.

There's no need to disable libid3tag support, it is present in Fedora (needs BR:
libid3tag-devel).

There's no reason to disable OSS support, either (no additional BRs or Requires:)

/etc/X11/wmconfig/opencubicplayer

What's that doing here? I thought these were long obsolete. Additionally, no
package owns /etc/X11/wmconfig anymore, so you'll leave unowned
/etc/X11/wmconfig directory after ocp is uninstalled.

# mv docs to docdir 
mkdir -p %{buildroot}%{_docdir}/%{name}-%{version} 
mv
%{buildroot}%{_datadir}/%{name}-%{version}/{AUTHORS,BUGS,COPYING,CREDITS,KEYBOARD_REMAPS,SUID,TODO}
\ 
   %{buildroot}%{_docdir}/%{name}-%{version} 

That's not how it's done. Just put those files in %doc.

mv %{buildroot}%{_datadir}/applications/opencubicplayer.desktop \ 
   %{buildroot}%{_datadir}/applications/fedora-ocp.desktop 

Not necessary. Just use --delete-original in desktop-file-install call.

Thanks.  I've applied most of your suggested changes.  New packages here:

Spec URL: http://cra.fedorapeople.org/ocp/ocp.spec
SRPM URL: http://cra.fedorapeople.org/ocp/ocp-0.1.15-2.fc9.src.rpm

Koji scratch builds:

f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=679020
f9: http://koji.fedoraproject.org/koji/taskinfo?taskID=679039

I've sent all patches upstream already.  I'll let upstream decide how to best
apply them and then I'll remove them from the package.  I'm told that the
libid3tag requirement is going away upstream, but I reenabled it anyway.  Not
sure it gets used without libmad being built...

Bumped release to 2 and will use only integers from now on.

I did try building x86_64 locally, and while it builds, there are issues with
popping/clipping and incorrect playback.  I've reenabled all archs for now and
will work with upstream as issues arise.

Regarding desktop-file-install, I'm renaming the file to meet the letter of this
guideline:

MUST: Packages containing GUI applications must include a %{name}.desktop file,
and that file must be properly installed with desktop-file-install in the
%install section. This is described in detail in the
[wiki:Self:Packaging/Guidelines#desktop desktop files section of Packaging
Guidelines] .

If you feel that using opencubicplayer.desktop rather than changing it to be
ocp.desktop is acceptable, I'll take out the rename.  It doesn't matter to me
either way...

Thanks again for taking a look at this.

Full review (relevant issues only)

$ rpmlint /var/lib/mock//fedora-development-i386/result
ocp.src: W: mixed-use-of-spaces-and-tabs (spaces: line 61, tab: line 61)
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Looks fine, that warning can be ignored.

Packaging Guidelines:

I'd rather you installed the icons as
/usr/share/icons/hicolor/{16x16,48x48}/apps/ocp.xpm
and used
Icon=ocp
in the desktop file instead of the full path. Or is the path /usr/share/pixmaps
hardcoded somewhere? This will of course require adding the proper gtk icon
cache update calls to %post(un).

Licencing is a bit problematic. Most files have just copyright assignment and no
licence header at all. Some have no licencing information whatsoever. Just
putting a COPYING file with GPL text in it in the tarball doesn't cut it. Please
work with upstream to fix that.

Source matches upstream:
b936f236b41e7f1184e401f5e099debe  ocp-0.1.15.tar.bz2

Builds fine on i386/devel and x86_64/F-8.

Everything else seems fine.

Ping?

I've asked upstream to add copyright information to all source files, and they
agreed that it was a good idea.  So now I'm just waiting to get a snapshot or
release with those changes.

Is it a strict blocker to not have license headers at the top of every file given that they do have copyright assignments and the tarball contains the COPYRIGHT file?  I've already worked with upstream and they agreed to put these in, but until they make a release with these changes, can this review request be approved?  I have the other change done already (icon file) but just haven't uploaded it yet.  Let me know and I'll upload the other changes.  Thanks.

No, it's not a blocker. And as you said, upstream is going to fix that anyway. Please go ahead and upload.

ping? I will sponsor you after this package passes review.

Ok, icon file change made.

Spec URL: http://cra.fedorapeople.org/ocp/ocp.spec
SRPM URL: http://cra.fedorapeople.org/ocp/ocp-0.1.15-3.fc10.src.rpm

Koji scratch build:
f9: http://koji.fedoraproject.org/koji/taskinfo?taskID=915017

Thanks.

missing Requires: hicolor-icon-theme (for the hicolor icon dirs)

...
# remove wmconfig stuff
rm -rf %{buildroot}/etc/X11/wmconfig/
rmdir %{buildroot}/etc/X11

These two could be shortened to:
rm -rf %{buildroot}/etc/X11

...
%{_bindir}/ultrafix.sh

This could be renamed to ocp-ultrafix.sh.

You can fix these issues after importing the package.

Package APPROVED and you are now sponsored.

Ah, I forgot: per current guidelines, you need to mention the status of the patches you apply in your package. AFAIK you said you'd sent them upstream, so please just add a comment above the PatchN: lines in the spec file to indicate that.

Also, I'm removing the FE-NEEDSPONSOR blocker.

New Package CVS Request
=======================
Package Name: ocp
Short Description: Open Cubic Player for MOD/S3M/XM/IT/SID/MIDI music files
Owners: cra
Branches: F-9 F-10
InitialCC:

cvs done.

Imported, requested changes made, and built for devel, F-10, F-9.  Closing bug.  Thanks guys.

New package reviews should be closed as NEXTRELEASE after importing.