Bug 238824 - Review Request: schismtracker - sound module composer/player
Review Request: schismtracker - sound module composer/player
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-03 03:09 EDT by Jindrich Novy
Modified: 2013-07-02 19:20 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-14 02:41:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
Fix remaining cosmetic issues (647 bytes, application/octet-stream)
2007-05-05 14:47 EDT, Ville Skyttä
no flags Details

  None (edit)
Description Jindrich Novy 2007-05-03 03:09:40 EDT
Spec URL: http://people.redhat.com/jnovy/files/schismtracker.spec
SRPM URL: http://people.redhat.com/jnovy/files/schismtracker-0.5-0.1rc1.fc7.src.rpm
Description:
Schismtracker is a module tracker for the X Window System similar to
the DOS program `Impulse Tracker'. Schismtracker can play/modify various
sound formats such as MOD, S3M, XM, IT, 669 and others.  The user interface
is mostly text-based using SDL for graphical output.
Comment 1 Ville Skyttä 2007-05-03 11:41:27 EDT
- MIME type associations missing from .desktop file, suggested:
    MimeType=audio/x-mod;audio/x-s3m;audio/x-stm;audio/x-it;audio/x-xm;
  Remember update-desktop-database too, see Packaging/ScriptletSnippets in Wiki

- Menu entry icon missing - install a bunch of different sizes from icons/
subdirs to /usr/share/icons/hicolor/*?

- What are the libXau-devel and libXdmcp-devel build dependencies used for?

- Missing build dependencies: libXt-devel, libXxf86misc-devel, libXv-devel

- %configure --disable-dependency-tracking for cleaner build output and possibly
a slight build speedup?

- Separate creation of the applications dir shouldn't be needed, I think
desktop-file-install takes care of it.

- Shouldn't COPYING.frag-opt and COPYING.libmodplug be included in %doc?
Comment 2 Jindrich Novy 2007-05-03 15:14:38 EDT
First, thanks for the review.


(In reply to comment #1)
> - MIME type associations missing from .desktop file, suggested:
>     MimeType=audio/x-mod;audio/x-s3m;audio/x-stm;audio/x-it;audio/x-xm;

Ok, I also added the audio/x-669-mod and audio/x-mtm.

>   Remember update-desktop-database too, see Packaging/ScriptletSnippets in Wiki

Done.

> - Menu entry icon missing - install a bunch of different sizes from icons/
> subdirs to /usr/share/icons/hicolor/*?
> 

Done.

> - What are the libXau-devel and libXdmcp-devel build dependencies used for?

ldd says schismtracker is dependent on them, so I listed them explicitely.

> - Missing build dependencies: libXt-devel, libXxf86misc-devel, libXv-devel

These seem to be brought by SDL-devel.

> - %configure --disable-dependency-tracking for cleaner build output and possibly
> a slight build speedup?

Done.

> - Separate creation of the applications dir shouldn't be needed, I think
> desktop-file-install takes care of it.

the useless install is now removed.

> - Shouldn't COPYING.frag-opt and COPYING.libmodplug be included in %doc?

Added.
Comment 3 Ville Skyttä 2007-05-03 17:08:18 EDT
(In reply to comment #2)

Please bump the release tag always when making changes to packages, even while
they're in review.  Makes it a less PITA to track progress and compare changes.

Regarding the release tag, the common form for pre-release packages is eg.
0.X.rc1, not 0.Xrc1 as in this package.  Not a big deal though, but I'd suggest
changing it.

> > - MIME type associations missing from .desktop file, suggested:
> >     MimeType=audio/x-mod;audio/x-s3m;audio/x-stm;audio/x-it;audio/x-xm;
> Ok, I also added the audio/x-669-mod and audio/x-mtm.

This seems to have slipped the SRPM build, there's no MimeType included in the
*.desktop in it.

> > - Menu entry icon missing - install a bunch of different sizes from icons/
> > subdirs to /usr/share/icons/hicolor/*?
> Done.

Ok, but %{_datadir}/icons/* results in ownership of a lot of dirs commonly owned
by other packages.  Does any of this package's dependencies bring in
hicolor-icon-theme?  If yes, the dir/file ownership should be changed to
something like %{_datadir}/icons/hicolor/*x*/apps/%{name}.png

"Icon=%{name}" or something similar should be added to the desktop file.

gtk-update-icon-cache missing, see
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

> > - What are the libXau-devel and libXdmcp-devel build dependencies used for?
> ldd says schismtracker is dependent on them, so I listed them explicitely.

My ldd on FC6 x86_64 doesn't, and I don't see anything in the sources that would
 use them.  Which distro version and arch did you use for building your package
that shows those deps?  Perhaps there's something causing unneeded lib
dependency bloat there.

> > - Missing build dependencies: libXt-devel, libXxf86misc-devel, libXv-devel
> These seem to be brought by SDL-devel.

Not on FC6 x86_64.
Comment 4 Jindrich Novy 2007-05-04 02:44:10 EDT
Updated. I'm using F7t4 on x86_64. Surprisingly it builds in mock successfully
even without any deps to X, so that the SDL-devel BuildRequire seems to be
sufficient.

I put the updated packages to:
http://people.redhat.com/jnovy/files/schismtracker/

so that you can have a look also at the x86_64 binary built in mock.
Comment 5 Ville Skyttä 2007-05-04 06:36:31 EDT
(In reply to comment #4)
> Updated. I'm using F7t4 on x86_64. Surprisingly it builds in mock successfully
> even without any deps to X, so that the SDL-devel BuildRequire seems to be
> sufficient.

It is, but doing so leaves support for X, Xkb and Xv out (grep for USE_X in the
source to see what that results in).  Is this really desirable?  If yes, X
should be disabled explicitly by passing --without-x to configure for
reproducible builds.  Otherwise, adding the build deps mentioned in comment 1
would get all X support built in.

The "too many dirs owned" problem mentioned in comment 3 persists.  Now that the
hicolor-icon-theme dep is there, the icon stuff in %files should be changed to
eg. %{_datadir}/icons/hicolor/*x*/apps/%{name}.*

"Name=Schism Tracker" would be better than "Name=Schismtracker" (to match the
window title) in the .desktop file.

.desktop Categories: Application (invalid) and X-Fedora (unused/unneeded) should
be dropped, and "Audio" could be added in addition to "AudioVideo". 
http://standards.freedesktop.org/menu-spec/latest/apa.html
Comment 6 Ville Skyttä 2007-05-04 06:37:59 EDT
(In reply to comment #5)
> eg. %{_datadir}/icons/hicolor/*x*/apps/%{name}.*

Hm, actually %{_datadir}/icons/hicolor/*/apps/%{name}.* so that it matches the
scalable dir too.
Comment 7 Jindrich Novy 2007-05-04 08:06:20 EDT
Ok, updated. I set the X dependencies to be optional and enabled them by default.
Comment 8 Ville Skyttä 2007-05-05 14:47:26 EDT
Created attachment 154210 [details]
Fix remaining cosmetic issues

Ok, 0.3 is looks good, approved.  Remaining cosmetic issues to fix/consider
before the first build:

- rpmlint:
  W: schismtracker mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 43)

- s/Skitta/Skyttä/ in the 0.5-0.3.rc1 changelog entry

- Use "0%{!?_without_x:1}" instead of "%{WITH_X}" so X can be disabled using
  "rpmbuild --without x ...".

Patch for these attached, gzipped so Bugzilla won't mangle UTF-8.
Comment 9 Jindrich Novy 2007-05-06 02:52:45 EDT
New Package CVS Request
=======================
Package Name: schismtracker
Short Description: Sound module composer/player
Owners: jnovy@fedoraproject.org
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC:
Comment 10 Warren Togami 2007-05-06 19:38:22 EDT
Is this really appropriate for EPEL?
Do you intend on maintaining it for 6.5 years?

branching only Fedora, if you *really* want it in EPEL do another fedora-cvs
request.

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