Fedora Merge Review: timidity++ http://cvs.fedora.redhat.com/viewcvs/devel/timidity++/ Initial Owner: twoerner
cc-ing Jindrich and Hans, assigning to myself.
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 -------------------------------------------------------------
(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.
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/
(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%).
Well, freepats is better than nothing, I suppose. Blocking FE-Legal.
Committed most of the proposed review fixes.
(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.
(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+.
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 :)
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?
(For people watching this Merge Review, please also check bug 430417) FE-Legal removed.
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.
(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.
Okay. -------------------------------------------------------------------- This package (timidity++) is APPROVED by me -------------------------------------------------------------------- Please rebuild PersonalCopy-Lite-soundfont (bug 430417) first for timidity++-patches obsoletes.
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).
p.s. Mamoru, I forgot to say this before: Many thanks for the review!