Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=494965 --- Comment #3 from Christian Krause <chkr@xxxxxxxxxxx> 2009-04-11 18:01:40 EDT --- (In reply to comment #1) > Here are my notes for this package (* need definite attention. ! are > suggestions): Thank you for the review. > * We need to package rtmidi seperately and link to it. I already packaged > rtaudio before. Things should be similar (you can take rtaudio SPEC file as > your starting point) I've checked the upstream rtmidi package ( http://www.music.mcgill.ca/~gary/rtmidi/ ) but unfortunately the package doesn't provide any Makefiles so that it is not easily possible to build a library out of the box. Since other projects also use rtmidi just by adding the 3 files, I would vote for not creating an extra library for rtmidi. Once rtmidi's upstream will provide a full-featured package to build dynamic (or static) libs this can be re-considered. > ! Please make use of the %{name} macro. Done. Besides the URL tag the specfile was changed to use the %{name} tag. > ! I find it better to supply the .desktop file separately, to preserve the > original creation date. But that's a matter of taste. Done. I've added the desktop file as separate source file. > ! I prefer using sed+iconv instead of dos2unix to save a BR. But then again > this is a matter of taste. If you are going to use dos2unix, could you use the > -k flags to preserve timestamps? Done. (Using sed now. However I haven't found an option to preserve the timestamp when using sed...) > * Similarly, please use the -p flag with install to preserve the timestamps (of > the .png files in this case) Done. > ! Please add a GenericName to the .desktop file. Also AudioVideo needs to be > added to the Category key, if you want this application to appear in Multimedia > group. Done. I've used your suggested "Piano Teacher" as GenericName and added the AudioVideo category. > * There is a typo on the installation of the 64x64 icon. (32x32 should be > 64x64) Done. > * Parallel make must be supported whenever possible. If it is not supported, > this should be noted in the SPEC file as a comment. Done. Changed to parallel build. I've seen no problems so far (in local as well as in koji/mock builds). > ! Summary seemed too long to me. It can be just something like "Piano Teacher" I've checked other specfiles too and I've seen a bunch of packages with similar summaries. I would rather leave the summary as it is. However, I've re-used the suggestion as the GenericName in the .desktop file. ;-) > ! I think the comma should be removed from the end of this line (for proper > English): > "The difference between playing along to a CD or a standard MIDI file," Done. The modified packages can be found here: Spec URL: http://chkr.fedorapeople.org/review/pianobooster.spec SRPM URL: http://chkr.fedorapeople.org/review/pianobooster-0.6.2-3.fc10.src.rpm -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review