Bug 238824

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

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.

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

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.

(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

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

Ok, updated. I set the X dependencies to be optional and enabled them by default.

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.

New Package CVS Request
=======================
Package Name: schismtracker
Short Description: Sound module composer/player
Owners: jnovy
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC:

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.