Bug 226492 - Merge Review: timidity++
Merge Review: timidity++
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On: 430417
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:10 EST by Nobody's working on this, feel free to take it
Modified: 2008-03-04 16:06 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-04 16:05:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:10:43 EST
Fedora Merge Review: timidity++

http://cvs.fedora.redhat.com/viewcvs/devel/timidity++/
Initial Owner: twoerner@redhat.com
Comment 1 Mamoru TASAKA 2008-01-23 11:47:06 EST
cc-ing Jindrich and Hans, assigning to myself.
Comment 2 Mamoru TASAKA 2008-01-23 13:19:14 EST
License check list:
===========================================================
Source:	GPLv2+

interface/VTparse.h	MIT
interface/VTPrsTbl.c	MIT
interface/xaw_redef.c	MIT
timidity/mt19937ar.c	BSD
utils/fft4g.c		Okay!
utils/fft4g.h		Okay!
   http://www.kurims.kyoto-u.ac.jp/~ooura/fft-j.html

Source1,3,4: SourceURL links broken, and all binaries.
             What are these files?
             Maybe Public Domain? or like
             "Redistributable, no modification permitted"?
===========================================================

For 2.13.2-6:

* Documents
  - Please add some documents to %doc
    * At least "COPYING" must be added
    * And consider to add some other documents such as
----------------------------------------------------------
TiMidity++-2.13.2/AUTHORS
TiMidity++-2.13.2/ChangeLog*
TiMidity++-2.13.2/README* 
   (TiMidity++-2.13.2/README.ja is encoded in EUC-JP)
----------------------------------------------------------
    * Also please check man files
----------------------------------------------------------
TiMidity++-2.13.2/doc/ja_JP.eucJP/timidity.1 (EUC-JP)
                                  timidity.cfg.5 
                                             (Same as above)
----------------------------------------------------------
      ! Note
        If you are willing to support Japanese man page,
        it
        - must be converted from EUC-JP to UTF-8 (by iconv, for
          example)
        - must be placed on %_mandir/ja/man1/timidity.1*
        - and must be marked as %lang(ja).

* Obsoletes
  - Please change Obsoletes to at least version specific
    (and why is this Obsoletes needed?)

* Parallel make
  - Please check if parallel make is possible.

* Timestamps
  - When using "cp" or "install" commands, please add "-p" option
    to keep timestamps.

* Verbose output
  - Any reason you want to use tar x"v"jf? (note that for %setup
    Fedora requests that it should be quiet)

* Hardcorded /etc
  - Is the reason you are using hardcorded /etc is the existence of
    Patch(0)? If so I was told to remove hardcoded path
    by like below:
-------------------------------------------------------------
%setup -q -n TiMidity++-%{version}
%Patch
.........
#  Ensure that we are actually using %%_sysconfdir
sed -i.path -e 's|/etc/timidity\.cfg|%{_sysconfdir}/timidity.cfg|' \
	timidity/timidity.h

%build
-------------------------------------------------------------
    and replace all /etc with %_sysconfdir. I also think
    this is better.
    Or maybe it can be replaced by below?
--------------------------------------------------------------
export EXTRACFLAGS="$RPM_OPT_FLAGS -DCONFIG_FILE=\"%{_sysconfdir}/timidity.cfg\""
--------------------------------------------------------------

* rpmlint
  - Only one thing:
-------------------------------------------------------------
timidity++-patches.i386: W: symlink-should-be-relative
/usr/share/timidity/timidity.cfg /etc/timidity.cfg
-------------------------------------------------------------
Comment 3 Hans de Goede 2008-01-24 11:11:20 EST
(In reply to comment #2)
> License check list:
> Source1,3,4: SourceURL links broken, and all binaries.
>              What are these files?
>              Maybe Public Domain? or like
>              "Redistributable, no modification permitted"?


Bummer,

I've spend some time (lots of time) investigating this instruments.tar.bz2
contains the midia (a public domain SGI midi player) patch set which is still
available here:
ftp://ftp.funet.fi/pub/sci/audio/softsynths/midia/

Its a .tar.gz there, but after unpacking both are identical. There is no clear
proof of this being public domain, other then it being allegedly pd here:
http://www.opensound.com/softoss.html

Notice that opensound is not distributing it anymore themselves, nor can the
complete midia player be found anywhere on the net.

I did eventually find this post where the author claims that the midia patches
are modified copies of the patches distributed with the gravis ultrasound and
thus are illegal:
http://archive.netbsd.se/?ml=netbsd-tech-pkg&a=2006-10&t=2429714

Taking the disappearance of midia from the net into account this doesn't sound
unbelievable.

...

So this leaves us with no patches and thus no midi playback capability, I've
just checked Debian, and they have the same problem.

I've spend hours browsing the net for reasonably sized General Midi patch sets,
none can be found in GUS .pat format, but I've found several in .sf2 format
(which timidity can use too, we may need to convert it though for use with
SDL_mixer / allegro / libtimidity / wildmidi).

All .sf2 GM patch sets I've found come either without a license statement, or
one which is not compatible with Fedora :(

However I've send mail to all patchset authors that I could find a working mail
address of, asking them for license conditions if none were given, and if they
were given if they are willing to reword things so that they become compatible
with Fedora's content guidelines.

I've send 6 mails in total, so stay tuned.

Since timidity has been part of RedHat since RedHat 6.2, presumably with these
patches, I'm currently not planning on removing them immediately, hoping that we
will have a backup for them in place soon, I'll send a private mail to Spot
pointing him to this bug.

Comment 4 Mamoru TASAKA 2008-01-24 12:13:51 EST
Well, I don't know what "patch"es should exist to make use of
timidity++, however is the following URL useful?

http://freepats.opensrc.org/
Comment 5 Hans de Goede 2008-01-24 13:39:40 EST
(In reply to comment #4)
> Well, I don't know what "patch"es should exist to make use of
> timidity++, however is the following URL useful?
> 
> http://freepats.opensrc.org/

Thanks for thinking along, but no, unfortunately that patchset does not give
complete General Midi coverage (it only covers about 50%).
Comment 6 Tom "spot" Callaway 2008-01-24 15:21:42 EST
Well, freepats is better than nothing, I suppose. Blocking FE-Legal.
Comment 7 Jindrich Novy 2008-01-25 05:35:40 EST
Committed most of the proposed review fixes.
Comment 8 Hans de Goede 2008-01-25 06:07:59 EST
(In reply to comment #7)
> Committed most of the proposed review fixes.

Thanks,

2 comments:

1) is jack-audio-connection-kit-devel available on ppc64 now? I added those
   hacks because it wasn't.
2) The GPLv2 license trumps / encompasses / overrules the MIT an BSD licenses,
   so the resulting binary is effectively licensed under the GPLv2 only.
Comment 9 Mamoru TASAKA 2008-01-25 06:14:51 EST
(In reply to comment #8)
> 2) The GPLv2 license trumps / encompasses / overrules the MIT an BSD licenses,
>    so the resulting binary is effectively licensed under the GPLv2 only.

As far as I checked the codes of timidity+, the license should be
GPLv2+.

Comment 10 Jindrich Novy 2008-01-25 06:18:02 EST
Hans, it seems that it still won't build on ppc64 with jack enabled:

http://koji.fedoraproject.org/koji/taskinfo?taskID=371942

So I reverted back to the ppc64 hack, we can try it again after some time :)
Comment 11 Hans de Goede 2008-01-27 17:18:03 EST
Good news after lots of searching and negotiating with the author I've managed
to find a soundfont that meets the content Guidelines for Fedora. I've already
send the license past Spot and he has approved it.

I've packages it and submitted it for review. Its review is bug 430417. Note
that in order to convert the original .sf2 file to gus patch format (needed by
SDL_mixer, allegro, libtimidity and wildmidi) a conversion utility is needed,
its review is bug 430418. So if someone can give me a hand and review these can
then we can get rid of the not licensed gus patches we are currently using.

If you look at soundfont package, you will see that the spec file generates 2
packages:
PersonalCopy-Lite-soundfont
PersonalCopy-Lite-patches

The first contains the pristine .sf2 file from upstream, the other contains the
same file converted into GUS patch format. Timidity++ can handle both, and seems
to do somehat better with the .sf2 format (for one timidity does not support
multiple velocity layers when reading the patches in the gus format).

So it would be best to make timidity depend upon the .sf2 format version.
However that brings us to the question what todo with /etc/timidity.cfg, as
already said: SDL_mixer, allegro, libtimidity and wildmidi all 4 only support
the gus format, and they expect to be able to read a generalmidi mapping in gus
format from /etc/timidity.cfg.

So my proposed solution is to let the gus format package install
/etc/timidity.cfg (and provide timidity++-patches, as that is required by the 4
listed above), and let the .sf2 format package install /etc/timidity-soundfont.cfg.

I then combine this with a patch to timidity to first check for
/etc/timidity-soundfont.conf and fallback to /etc/timidity.cfg if that is
missing, and all is well. Any opinions / input on this?
Comment 12 Mamoru TASAKA 2008-03-03 11:03:13 EST
(For people watching this Merge Review, please also check
 bug 430417)

FE-Legal removed.
Comment 13 Mamoru TASAKA 2008-03-03 11:29:10 EST
For 2.13.2-13:

* Patches merge
  - Please merge the patches which can be merged (especially
    a patch and a patch to the patch, e.g. Patch(0) and
    Patch16)

* License
  - Now License tag is just GPLv2.

* Macros
  - Unify macros usage. For example (I have not checked all
    macros usage inconsistency)
-------------------------------------------------------
echo "soundfont /usr/share/soundfonts/PCLite.sf2" > \
  $RPM_BUILD_ROOT/%{_sysconfdir}/timidity++.cfg
......
%config(noreplace) /etc/timidity++.cfg
-------------------------------------------------------
echo "soundfont /usr/share/soundfonts/PCLite.sf2" > \
  $RPM_BUILD_ROOT/%{_sysconfdir}/timidity++.cfg

(From PersonalCopy-Lite-soundfont:)
%{_datadir}/soundfonts
-------------------------------------------------------

  - For hardcoded /etc, please also check my comment 2.
Comment 14 Hans de Goede 2008-03-03 15:17:00 EST
(In reply to comment #13)
> For 2.13.2-13:
> 
> * Patches merge
>   - Please merge the patches which can be merged (especially
>     a patch and a patch to the patch, e.g. Patch(0) and
>     Patch16)
> 
> * License
>   - Now License tag is just GPLv2.
> 
> * Macros
>   - Unify macros usage. For example (I have not checked all
>     macros usage inconsistency)

All fixed.

>   - For hardcoded /etc, please also check my comment 2.

Erm, asking consistent use of %{_sysconfdir} in a .spec I agree with, but asking
to use rpm macros in patches, thats just pedantic: -ewontfix

New version with the 3 items above fixed:
http://people.atrpms.net/~hdegoede/timidity++.spec
http://people.atrpms.net/~hdegoede/timidity++-2.13.2-13.fc9.src.rpm

Note to people watching along this is not build / committed to CVS yet as it
requires PersonalCopy-Lite-soundfont which is under review, see bug 430417.
Comment 15 Mamoru TASAKA 2008-03-04 05:36:36 EST
Okay.

--------------------------------------------------------------------
    This package (timidity++) is APPROVED by me
--------------------------------------------------------------------

Please rebuild PersonalCopy-Lite-soundfont (bug 430417) first
for timidity++-patches obsoletes.
Comment 16 Hans de Goede 2008-03-04 16:03:09 EST
PersonalCopy-Lite-soundfont imported and build. propsed new timidity++ package
committed and build in rawhide, closing.

Note I will also be builing both packages as an update for F-7 and F-8 to
resolve the license issues there too (by request of Spot).
Comment 17 Hans de Goede 2008-03-04 16:06:33 EST
p.s.

Mamoru, I forgot to say this before:

Many thanks for the review!

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