Bug 452749 - (ocp) Review Request: ocp - Open Cubic Player for MOD/S3M/XM/IT/SID/MIDI music files
Review Request: ocp - Open Cubic Player for MOD/S3M/XM/IT/SID/MIDI music files
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominik 'Rathann' Mierzejewski
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2008-06-24 15:42 EDT by Charles R. Anderson
Modified: 2009-03-22 20:16 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-11-10 20:25:59 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dominik: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Charles R. Anderson 2008-06-24 15:42:34 EDT
Spec URL: http://cra.fedorapeople.org/ocp/ocp.spec
SRPM URL: http://cra.fedorapeople.org/ocp/ocp-0.1.15-1.2.fc9.src.rpm
Description: Open Cubic Player is a music file player ported from DOS that supports Amiga MOD module formats and many variants, such as MTM, STM, 669, S3M, XM, and IT.  It is also able to render MIDI files using sound patches and play SID, OGG Vorbis, and WAV files.  OCP provides a nice text-based interface with several text-based and graphical visualizations.

Koji scratch builds:

f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=678736
f9: http://koji.fedoraproject.org/koji/taskinfo?taskID=678694

This is my first package submission, so I need a sponsor.  Thanks!!
Comment 1 Dominik 'Rathann' Mierzejewski 2008-06-24 16:20:44 EDT
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?
Comment 2 Charles R. Anderson 2008-06-24 16:52:45 EDT
My FAS user name is cra.  I'd be glad to look at some of your packages.  Thanks.
Comment 3 Dominik 'Rathann' Mierzejewski 2008-06-24 18:16:38 EDT
Partial review::

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

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:

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


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} 

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

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

Not necessary. Just use --delete-original in desktop-file-install call.
Comment 4 Charles R. Anderson 2008-06-24 21:13:44 EDT
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

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.
Comment 5 Dominik 'Rathann' Mierzejewski 2008-07-07 13:40:36 EDT
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
and used
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.
Comment 6 Dominik 'Rathann' Mierzejewski 2008-07-28 07:58:48 EDT
Comment 7 Charles R. Anderson 2008-07-28 10:41:47 EDT
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.
Comment 8 Charles R. Anderson 2008-10-11 11:29:58 EDT
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.
Comment 9 Dominik 'Rathann' Mierzejewski 2008-10-13 13:43:35 EDT
No, it's not a blocker. And as you said, upstream is going to fix that anyway. Please go ahead and upload.
Comment 10 Dominik 'Rathann' Mierzejewski 2008-11-02 11:11:11 EST
ping? I will sponsor you after this package passes review.
Comment 11 Charles R. Anderson 2008-11-02 15:13:25 EST
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

Comment 12 Dominik 'Rathann' Mierzejewski 2008-11-07 11:01:59 EST
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


This could be renamed to ocp-ultrafix.sh.

You can fix these issues after importing the package.

Package APPROVED and you are now sponsored.
Comment 13 Dominik 'Rathann' Mierzejewski 2008-11-07 11:25:22 EST
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.
Comment 14 Charles R. Anderson 2008-11-08 00:46:09 EST
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
Comment 15 Kevin Fenzi 2008-11-10 11:53:11 EST
cvs done.
Comment 16 Charles R. Anderson 2008-11-10 20:25:59 EST
Imported, requested changes made, and built for devel, F-10, F-9.  Closing bug.  Thanks guys.
Comment 17 Dominik 'Rathann' Mierzejewski 2009-03-22 20:16:15 EDT
New package reviews should be closed as NEXTRELEASE after importing.

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