Bug 248119

Summary:	Review Request: libtimidity - MIDI to WAVE converter library
Product:	[Fedora] Fedora	Reporter:	Hans de Goede <hdegoede>
Component:	Package Review	Assignee:	Mamoru TASAKA <mtasaka>
Status:	CLOSED NEXTRELEASE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	fedora-package-review, notting
Target Milestone:	---	Flags:	mtasaka: fedora-review+ wtogami: fedora-cvs+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2007-07-24 18:58:52 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Hans de Goede 2007-07-13 12:00:43 UTC

Spec URL: http://people.atrpms.net/~hdegoede/libtimidity.spec
SRPM URL: http://people.atrpms.net/~hdegoede/libtimidity-0.1.0-1.fc8.src.rpm
Description:
This library is based on the TiMidity decoder from SDL_sound library.
Purpose to create this library is to avoid unnecessary dependences.
SDL_sound requires SDL and some other libraries, that not needed to
process MIDI files. In addition libtimidity provides more suitable
API to work with MIDI songs, it enables to specify full path to the
timidity configuration file, and have function to retrieve meta data
from MIDI song.


---

Reviewers: note, this is necessary to add midi playback support to gstreamer, since the plugin is currently under development it is in gstreamer-plugins-bad from the repo that must not be named. To test this, install libtimidity-devel and the gstreamer-plugins-bad.src.rpm and rebuild gstreamer-plugins-bad.

Comment 1 Mamoru TASAKA 2007-07-23 14:57:59 UTC

For 0.1.0-1:

* License
  - While COPYING says this is licensed under LGPL (and you
    tagged this rpm as such, although COPYING is missing on
    %doc entry), common.c, instrum.c, etc... are licensed 
    under _GPL_ .

? Dependency
  - Would you explain why main package should explicitly have
    "Requires: timidity++"?

? binary executable files
  - Are test/.libs/{playmidi,midi2raw} of no use?
    (playmidi seems to be created only with have libao-devel
     installed).

* libtimidity.pc
  - Why does this need "-lm" on Libs? Linkage against libm.so
    is already done on libtimidity.so and extra linkage like
    this must be removed unless header files installed require
    it.

Comment 2 Hans de Goede 2007-07-23 15:18:15 UTC

(In reply to comment #1)
> For 0.1.0-1:
> 
> * License
>   - While COPYING says this is licensed under LGPL (and you
>     tagged this rpm as such, although COPYING is missing on
>     %doc entry), common.c, instrum.c, etc... are licensed 
>     under _GPL_ .
> 

These files come from timidity (through SDL_mixer, which also is LGPL), when
timidity was discontinued it was set free under a choice if GPL / LGPL /
artistic, see:
http://web.archive.org/web/20010106150700/http://www.cgs.fi/~tt/discontinued.html

Also see the first paragraph of the included COPYING which says:

Please note that the included source from TiMidity, is also licensed
 under the following terms (GNU LGPL), but can also be used separately
 under the GNU GPL, or the Perl Artistic License. Those licensing
 terms are not reprinted here, but can be found on the web easily.

I will include the missing COPYING file, good catch.

> ? Dependency
>   - Would you explain why main package should explicitly have
>     "Requires: timidity++"?
> 

Because timidity++ provides the necessary patch files containing the wavetable
instruments, this is why for example SDL_mixer and allegro also require timidity++.

> ? binary executable files
>   - Are test/.libs/{playmidi,midi2raw} of no use?
>     (playmidi seems to be created only with have libao-devel
>      installed).
> 

There already is a cmdline utility called playmidi (or atleast was), also we
already have playmus from SDL_mixer and timidity itself through timidity++,
notice that the standalone version of timidity is of much better quality (and
much more cpu intensive). So I see little use in these test apps, if people want
to play or convert midi using timidity they should be using the standalone version.


> * libtimidity.pc
>   - Why does this need "-lm" on Libs? Linkage against libm.so
>     is already done on libtimidity.so and extra linkage like
>     this must be removed unless header files installed require
>     it.

Many package config files contain libs in the LDFLAGS against which the lib
itself is already linked, this is normal behaviour as not all Unix OS's support
.so files having deps, so sometimes the app itself must be linked against the
deps, so that the symbols will be there for the lib.

Comment 3 Mamoru TASAKA 2007-07-23 16:36:35 UTC

All right.
Please add COPYING on CVS.

-------------------------------------------------------
  This package (libtimidity) is APPROVED by me
-------------------------------------------------------

Comment 4 Hans de Goede 2007-07-23 16:51:21 UTC

Thanks for the review!

New Package CVS Request
=======================
Package Name:      libtimidity
Short Description: MIDI to WAVE converter library
Owners:            j.w.r.degoede
Branches:          FC-6 F-7 devel
InitialCC:         <empty>

Comment 5 Hans de Goede 2007-07-24 18:58:52 UTC

Imported and build, closing.