[Bug 452749] Review Request: ocp - Open Cubic Player for MOD/S3M/XM/IT/SID/MIDI music files

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: ocp - Open Cubic Player for MOD/S3M/XM/IT/SID/MIDI music files


https://bugzilla.redhat.com/show_bug.cgi?id=452749





------- Additional Comments From rpm at greysector.net  2008-06-24 18:16 EST -------
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.


-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.