Bug 494965 - Review Request: pianobooster - A MIDI file player that teaches you how to play the piano
Review Request: pianobooster - A MIDI file player that teaches you how to pla...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-08 16:38 EDT by Christian Krause
Modified: 2009-05-08 23:58 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.6.2-4.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-21 20:49:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Christian Krause 2009-04-08 16:38:10 EDT
Spec URL: http://chkr.fedorapeople.org/review/pianobooster.spec
SRPM URL: http://chkr.fedorapeople.org/review/pianobooster-0.6.2-2.fc10.src.rpm

Description:
A MIDI file player/game that displays the musical notes AND teaches you how
to play the piano.
PianoBooster is a fun way of playing along with a musical accompaniment and
at the same time learning the basics of reading musical notation.
The difference between playing along to a CD or a standard MIDI file,
is that PianoBooster listens and reacts to what you are playing on a
MIDI keyboard.

- rpmlint SPECS/pianobooster.spec SRPMS/pianobooster-0.6.2-2.fc10.src.rpm RPMS/i386/pianobooster-0.6.2-2.fc10.i386.rpm RPMS/i386/pianobooster-debuginfo-0.6.2-2.fc10.i386.rpm 
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

- koji builds:
F10: https://koji.fedoraproject.org/koji/taskinfo?taskID=1285986
F9:https://koji.fedoraproject.org/koji/taskinfo?taskID=1285981
I could not build the package for rawhide today. It looks like that lots of other packages fail in F11 right now, too. The package built successfully in rawhide yesterday:
F11: https://koji.fedoraproject.org/koji/taskinfo?taskID=1284073

- please note that the packages built in koji can't be used directly in F10 or F9 since there is an incompatible update of qt4 in updates-testing which has the build-f10-overrides tag set
Comment 1 Orcan Ogetbil 2009-04-08 21:02:10 EDT
Here are my notes for this package (* need definite attention. ! are suggestions):

* 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)

Note that rtmidi is MIT, but it is compatible with GPL.

! Please make use of the %{name} macro.

! I find it better to supply the .desktop file separately, to preserve the original creation date. But that's a matter of taste.

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

* Similarly, please use the -p flag with install to preserve the timestamps (of the .png files in this case)

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

* There is a typo on the installation of the 64x64 icon. (32x32 should be 64x64)

* Parallel make must be supported whenever possible. If it is not supported, this should be noted in the SPEC file as a comment.

! Summary seemed too long to me. It can be just something like "Piano Teacher"

! 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,"
Comment 2 Christian Krause 2009-04-11 15:18:48 EDT
I've found license problem in this package:

The source (gplv3+) contains 3 files ("rtmidi") which are licensed under a modified MIT license. In general this would be ok, but this license contains the statement that modifications must be sent back to the author.

I'll let this bug block FE-LEGAL to get their advice.

Here is the complete license:
    RtMidi WWW site: http://music.mcgill.ca/~gary/rtmidi/

    RtMidi: realtime MIDI i/o C++ classes
    Copyright (c) 2003-2009 Gary P. Scavone

    Permission is hereby granted, free of charge, to any person
    obtaining a copy of this software and associated documentation files
    (the "Software"), to deal in the Software without restriction,
    including without limitation the rights to use, copy, modify, merge,
    publish, distribute, sublicense, and/or sell copies of the Software,
    and to permit persons to whom the Software is furnished to do so,
    subject to the following conditions:

    The above copyright notice and this permission notice shall be
    included in all copies or substantial portions of the Software.

    Any person wishing to distribute modifications to the Software is
    requested to send the modifications to the original developer so that
    they can be incorporated into the canonical version.

    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
    EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
    MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
    IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR
    ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF
    CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
    WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
Comment 3 Christian Krause 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
Comment 4 Orcan Ogetbil 2009-04-12 04:10:05 EDT
Thanks for the update.

(In reply to comment #3)
> (In reply to comment #1)
> 
> > ! 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...)
> 

The standard way of making this is:

sed -e 's|\r||g' file > file.tmp
touch -r file file.tmp
mv file.tmp file


Let's wait for FE Legal's resolution for the license issue.
Comment 5 Tom "spot" Callaway 2009-04-14 14:14:05 EDT
The additional clause in the MIT license is a request, not a requirement. Accordingly, it is not a problem. 

Since the MIT code is compiled together with GPLv3+ code, you do not need to mention it in the License tag (the GPLv3+ terms are more restrictive), unless you wish to do so. License: GPLv3+ is fine. Lifting FE-Legal.
Comment 6 Orcan Ogetbil 2009-04-14 14:21:15 EDT
OK, since we don't have a legal problem we can approve this package:

-----------------------------------------------
This package (pianobooster) is APPROVED by oget
-----------------------------------------------
Comment 7 Christian Krause 2009-04-14 14:27:30 EDT
New Package CVS Request
=======================
Package Name: pianobooster
Short Description: A MIDI file player that teaches you how to play the piano
Owners: chkr
Branches: F10
InitialCC:
Comment 8 Kevin Fenzi 2009-04-16 00:29:50 EDT
I assume you want a F-11 branch as well here. 

cvs done with an F-11 branch too.
Comment 9 Fedora Update System 2009-04-20 15:08:30 EDT
pianobooster-0.6.2-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/pianobooster-0.6.2-4.fc10
Comment 10 Fedora Update System 2009-04-20 15:27:31 EDT
pianobooster-0.6.2-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/pianobooster-0.6.2-4.fc11
Comment 11 Fedora Update System 2009-04-21 20:49:00 EDT
pianobooster-0.6.2-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 12 Fedora Update System 2009-05-08 23:58:25 EDT
pianobooster-0.6.2-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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