Bug 510668 - Review Request: canorus - Music Score Editor
Review Request: canorus - Music Score Editor
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christian Krause
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-10 03:15 EDT by Orcan Ogetbil
Modified: 2009-10-08 23:43 EDT (History)
5 users (show)

See Also:
Fixed In Version: 0.7-4.R1177.20090904svn.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-08 23:41:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
chkr: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Orcan Ogetbil 2009-07-10 03:15:53 EDT
Spec URL: http://oget.fedorapeople.org/review/canorus.spec
SRPM URL: http://oget.fedorapeople.org/review/canorus-0.7-1.R1163.fc11.src.rpm
Description: 
Canorus is a free extensible music score editor. It supports note writing,
import/export of various file formats, MIDI input and output, scripting and
more! Using a Qt4 framework Canorus offers a fast and modern GUI and
cross-platformability.

rpmlints:
   6 W: dangling-relative-symlink warnings
These are satisfied through font dependencies and can be ignored.

koji rawhide build:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=1465031
Comment 1 Susi Lehtola 2009-07-10 15:55:08 EDT
Just a few drive-by comments:

- Shouldn't the menu image go to %{_datadir}/pixmaps? Then you could drop the hicolor stuff.

- Is the %{_datadir}/%{name}/doc directory accessed by the executable? Is the documentation in that directory human-readable? If yes, I suggest putting the doc/ directory in %doc and replacing %{_datadir}/%{name}/doc with a symlink to %{_docdir}/%{name}-%{version}/doc/, thus simplifying
 %doc %{_datadir}/%{name}/doc/
 %dir %{_datadir}/%{name}/
 %{_datadir}/%{name}/examples/
 %{_datadir}/%{name}/fonts/
 %{_datadir}/%{name}/images/
 %{_datadir}/%{name}/plugins/
 %{_datadir}/%{name}/scripts/
to
 %doc doc/ (or whatever is the location of the docs in the source tree)
 %{_datadir}/%{name}/

- The install command
 DESTDIR=%{buildroot} make install DESTDIR=%{buildroot}
has one DESTDIR too much :) You should be able to drop the former one.
Comment 2 Orcan Ogetbil 2009-07-10 16:12:48 EDT
(In reply to comment #1)
> Just a few drive-by comments:
> 
> - Shouldn't the menu image go to %{_datadir}/pixmaps? Then you could drop the
> hicolor stuff.
> 

I once heard that %{_datadir}/pixmaps is being deprecated, and new stuff should go into the hicolor directory.

> - Is the %{_datadir}/%{name}/doc directory accessed by the executable? Is the
> documentation in that directory human-readable? If yes, I suggest putting the
> doc/ directory in %doc and replacing %{_datadir}/%{name}/doc with a symlink to
> %{_docdir}/%{name}-%{version}/doc/, thus simplifying
>  %doc %{_datadir}/%{name}/doc/
>  %dir %{_datadir}/%{name}/
>  %{_datadir}/%{name}/examples/
>  %{_datadir}/%{name}/fonts/
>  %{_datadir}/%{name}/images/
>  %{_datadir}/%{name}/plugins/
>  %{_datadir}/%{name}/scripts/
> to
>  %doc doc/ (or whatever is the location of the docs in the source tree)
>  %{_datadir}/%{name}/
> 

The docs are in the Qt format and afaik they can't be read externally. They can be read through the software only, by going Help->User's guide

I can also build the PDF docs. But these will be identical to the Qt docs. And I think, from a users perspective, it is more convenient to access them through the software itself.

> - The install command
>  DESTDIR=%{buildroot} make install DESTDIR=%{buildroot}
> has one DESTDIR too much :) You should be able to drop the former one.  

Ah, yes. I was testing stuff and I forgot to clean that up. Thanks for pointing!
Comment 3 Orcan Ogetbil 2009-07-10 16:16:59 EDT
Oh, also I can't do the suggested simplification because there are also Qt locale files inside %{_datadir}/%{name}/lang/ that need to be marked with %lang($locale)
Comment 4 Orcan Ogetbil 2009-07-11 15:04:30 EDT
Spec URL: http://oget.fedorapeople.org/review/canorus.spec
SRPM URL: http://oget.fedorapeople.org/review/canorus-0.7-2.R1163.fc11.src.rpm

ChangeLog: 0.7-2.R1163
- Minor cleanup in %%install section
Comment 5 Luis Garrido 2009-07-12 18:51:40 EDT
Hey, Orcan. I have tried your SRPM in FC10, only substituting the font(*) packages for urw-fonts, lilypond and freefont, that provide the required fonts.

Package builds fine and the application can be launched, but the music symbols are kind of messed up (the G clef is depicted as a pedal mark, for instance.)

At first glance it appears the emmentaler font in lilypond is not exactly the one canorus expects.

Screenshot here:

http://tinypic.com/r/2hmzdig/3
Comment 6 Orcan Ogetbil 2009-07-12 19:25:51 EDT
Luis,
You need to make sure that the correct font files are symlinked.
Check:
   $ ll /usr/share/canorus/fonts/
If all the links are sane, things should display properly. They do on F-11.

Would you like me to make this package F-10 compatible?
Comment 7 Luis Garrido 2009-07-12 20:28:12 EDT
Ah, ok, I see.

Sure, please, it would be nice to have it in FC10.

You could tweak the postinst script to do something like:

# Handle fonts
rm -f %{buildroot}%{_datadir}/%{name}/fonts/*.ttf
lilyfontdir = ../../fonts/lilypond/`rpm -q --qf %{VERSION} lilypond`/fonts/otf
ln -s ../../fonts/freefont/FreeSans.ttf %{buildroot}%{_datadir}/%{name}/fonts/
ln -s $lilyfontdir/emmentaler-14.otf %{buildroot}%{_datadir}/%{name}/fonts/
ln -s $lilyfontdir/CenturySchL-Bold.otf %{buildroot}%{_datadir}/%{name}/fonts/
ln -s $lilyfontdir/CenturySchL-BoldItal.otf %{buildroot}%{_datadir}/%{name}/fonts/
ln -s $lilyfontdir/CenturySchL-Ital.otf %{buildroot}%{_datadir}/%{name}/fonts/
ln -s $lilyfontdir/CenturySchL-Roma.otf %{buildroot}%{_datadir}/%{name}/fonts/

This won't work because rpmbuild barfs at the assignment command, I tried it. Having to build the full package just to test small changes to the postinst stuff is beyond stupid, and I can't seem to find a shortcut that skips that stage, so every try takes 15 minutes. Any tip for rebuilding a RPM after changing small things like Requires or helper scripts without having to perform the whole routine?

Anyway, I remade the symbolic links manually and the symbols have changed, but they are still wrong, even if there are no broken links anymore. I also get an error when I invoke canorus from the command line:

Error: _CanorusPython.so not found
Comment 8 Orcan Ogetbil 2009-07-13 02:16:25 EDT
(In reply to comment #7)
> Ah, ok, I see.
> 
> Sure, please, it would be nice to have it in FC10.
> 
> You could tweak the postinst script to do something like:
> 
> # Handle fonts
> rm -f %{buildroot}%{_datadir}/%{name}/fonts/*.ttf
> lilyfontdir = ../../fonts/lilypond/`rpm -q --qf %{VERSION} lilypond`/fonts/otf
> ln -s ../../fonts/freefont/FreeSans.ttf %{buildroot}%{_datadir}/%{name}/fonts/
> ln -s $lilyfontdir/emmentaler-14.otf %{buildroot}%{_datadir}/%{name}/fonts/
> ln -s $lilyfontdir/CenturySchL-Bold.otf %{buildroot}%{_datadir}/%{name}/fonts/
> ln -s $lilyfontdir/CenturySchL-BoldItal.otf
> %{buildroot}%{_datadir}/%{name}/fonts/
> ln -s $lilyfontdir/CenturySchL-Ital.otf %{buildroot}%{_datadir}/%{name}/fonts/
> ln -s $lilyfontdir/CenturySchL-Roma.otf %{buildroot}%{_datadir}/%{name}/fonts/
> 
> This won't work because rpmbuild barfs at the assignment command, I tried it.

It doesn't work because of these spaces

lilyfontdir = ../../fonts/lilypond/`rpm -q --qf %{VERSION} lilypond`/fonts/otf
           ^ ^

It is also not a good practice to invoke "rpm" in a specfile. Just use *

> Having to build the full package just to test small changes to the postinst
> stuff is beyond stupid, and I can't seem to find a shortcut that skips that
> stage, so every try takes 15 minutes. Any tip for rebuilding a RPM after
> changing small things like Requires or helper scripts without having to 
> perform the whole routine?
> 

Not that I know of. If you are using mock, you can pass a --no-clean so that the previous mock root is used. That's pretty much it.

> Anyway, I remade the symbolic links manually and the symbols have changed, but
> they are still wrong, even if there are no broken links anymore. 

The application works here with no weird symbols. I asked nim-nim how I should handle fonts on F-10. Maybe it's okay to used the bundled fonts on F-10. But we'll see.

> I also get an error when I invoke canorus from the command line:
> 
> Error: _CanorusPython.so not found  

Short answer:
This can be ignored. 

Long answer:
The application tries to add 
   /usr/share/canorus/_CanorusPython.so
to syspath for python CLI. But we don't install that file there because it is a wrong place. No arch specific binary lib files should go to /usr/share/. I'm in contact with upstream to fix this for good.

Instead, we install this file to
   /usr/lib(64)/python2.6/site-packages/
which is already in syspath.
Comment 9 Orcan Ogetbil 2009-07-13 04:47:35 EDT
I see that the emmentaler-14 font in F-10 is bad. It is missing a glyph at 0xe19d and because of that, all glyphs coming after this one is shifted by one unit. (for example: if these were regular fonts, this means that you push "p" button but your computer displays a "q")

nim-nim told me that I can't use the bundled fonts on F-10 either, so this has to be fixed on the lilypond side.

For your own use, just remove the
   # Handle fonts
   ln -s ...
section entirely from the specfile. Also remove the system-fonts patch. The package should work then on F-10.

I will contact the lilypond author to see if he'll fix the package on F-10.
Comment 10 Luis Garrido 2009-07-13 08:57:40 EDT
> For your own use, just remove the
>    # Handle fonts
>    ln -s ...
> section entirely from the specfile. Also remove the system-fonts patch. The
> package should work then on F-10.
> 

Fix confirmed. Thanks!
Comment 11 Orcan Ogetbil 2009-07-15 03:08:24 EDT
lilypond is fixed in F-10 :) New packages will hit the testing repo soon.

Meanwhile this package still needs a reviewer :)
Comment 12 Christian Krause 2009-07-19 05:52:30 EDT
(In reply to comment #11)
> lilypond is fixed in F-10 :) New packages will hit the testing repo soon.
> 
> Meanwhile this package still needs a reviewer :)  

If nobody objects, I'll do the review.
Comment 13 Christian Krause 2009-07-26 06:35:58 EDT
Sorry for the delay, here is the complete review:

* rpmlint:  OK
rpmlint SPECS/canorus.spec RPMS/i586/canorus-* SRPMS/canorus-0.7-2.R1163.fc11.src.rpm 
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/emmentaler-14.otf ../../fonts/lilypond/emmentaler-14.otf
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/CenturySchL-BoldItal.otf ../../fonts/lilypond/CenturySchL-BoldItal.otf
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/FreeSans.ttf ../../fonts/gnu-free/FreeSans.ttf
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/CenturySchL-Roma.otf ../../fonts/lilypond/CenturySchL-Roma.otf
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/CenturySchL-Ital.otf ../../fonts/lilypond/CenturySchL-Ital.otf
canorus.i586: W: dangling-relative-symlink /usr/share/canorus/fonts/CenturySchL-Bold.otf ../../fonts/lilypond/CenturySchL-Bold.otf
3 packages and 1 specfiles checked; 0 errors, 6 warnings.

the reported errors are false positive since the link targets are provided
by packages listed as "Requires:"


* naming: TODO
- name matches upstream
- spec file name matches package name
- snapshot release tag (assuming it is a post-release snapshot) should contain the date (the svnrev can be appended, but the date is required)
( according to http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages )


* License: TODO
- the package contains sources under the GPLv2, too:
src/import/pmidi/except.c (GPLv2 as published), most likely this means
that the complete package must be released as GPLv2
- the source package (and the built binary package) contain lots of examples and so it is necessary to check their legal status - in the worst case they must not only be stripped out from the binary but also from the sources - do you have any information whether they are distributable?
- license file packaged: if the final package would be GPLv2, then we should not package GPLv1

* specfile in American English and legible: OK

* %description: OK

* Sources: OK
- Source0 URL ok
- spectool -g canorus.spec works
- sources matches upstream - md5sum:
2dc201fec21d781d0add487c5a9ed35b  canorus_0.7svn.R1163_source.tar.bz2
- even if the URL for the nightly builds is linked directly from canorus' homepage it is a little bit strange to use plain IP addresses here - let's hope that upstream does an official release soon and the URL can be changed again to something like this:
http://prdownload.berlios.de/canorus/canorus_0.7.R1002_source.tar.bz2

* Patches: OK
- probably Patch3 looks like that it could be accepted upstream

* Python requirements: 
- BR: python-devel: OK
- set python_sitearch when including arch-specific libraries: OK

* Compilation: OK (except F10)
- mock build works
- package builds correctly in koji for F12, F11 and F10
- however, due to the font problem, the package can't be installed in F10
- RPMOPTFLAGS used
- parallel build supported via _smp_mflags 

* debuginfo sub-package: OK
- non-empty
- debuginfo file works together with gdb

* BuildRequires: OK

* Locales handling: TODO
The package contains language files in a non-gettext format (*.qm files). 
Is it necessary to add them also via the %lang(xx) tag?

* shared/static libs, pkgconfig/header/*.la files: OK (n/a)

* packages must own all directories: OK

* files not listed twice: OK

* permissions of files: OK
- %defattr used
- final file permissions OK

* %clean section: OK

* macro usage: OK

* code vs. content: TODO
- see above (license issues)
- package contains example midi files, sheets of music, etc. (3.5 MB)
- according to the guidelines examples in general are not considered as
content, but if they e.g. contain notes from music which is still under
copyright I assume it would not be permissable...

* large documentation into subpackage: OK (n/a)

* GUI application needs %{name}.desktop: OK

* no directories owned which are already owned by other packages: OK

* rm -rf %{buildroot} at the beginning of %{install}: OK

* all file names UTF8: OK

* functional test: TODO
- program segfaults when it is closed
- program segfaults when opening any of the musicxml examples

* Scriptlets:
- icon cache: OK
- desktop database: OK (.desktop file contains MimeType)
Comment 14 Orcan Ogetbil 2009-07-26 16:28:40 EDT
Thanks for the extensive review! I fixed most of the issues and I contacted upstream and ask them about the license issues. I will update the SRPM according to their reply:
   https://lists.berlios.de/pipermail/canorus-devel/2009-July/000989.html

To make my understanding clear, even if the midi and musicxml files are free, can I not put them in the package?

The upstream is aware of the crashes (I talked to them on IRC). They say that the software is still at alpha state and most of these issues will be fixed in time. Whereas there are still nice things you can do with canorus, it should not be used professionally yet.
Comment 15 Christian Krause 2009-07-27 18:41:29 EDT
(In reply to comment #14)
> To make my understanding clear, even if the midi and musicxml files are free,
> can I not put them in the package?

Just for completeness:
https://www.redhat.com/archives/fedora-packaging/2009-July/msg00131.html
Comment 16 Orcan Ogetbil 2009-08-12 04:15:05 EDT
Sorry Christian, I have been really busy lately. I will get back to this in a week or two.
Comment 17 Orcan Ogetbil 2009-08-22 02:46:08 EDT
Yay! Finally! I'm very sorry for the delay

(In reply to comment #13)
> 
> * naming: TODO
> - name matches upstream
> - spec file name matches package name
> - snapshot release tag (assuming it is a post-release snapshot) should contain
> the date (the svnrev can be appended, but the date is required)
> ( according to
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages )
> 

Added

> 
> * License: TODO
> - the package contains sources under the GPLv2, too:
> src/import/pmidi/except.c (GPLv2 as published), most likely this means
> that the complete package must be released as GPLv2
> - the source package (and the built binary package) contain lots of examples
> and so it is necessary to check their legal status - in the worst case they
> must not only be stripped out from the binary but also from the sources - do
> you have any information whether they are distributable?
> - license file packaged: if the final package would be GPLv2, then we should
> not package GPLv1
> 

I removed the midi and the xml files which have unclear licenses. Also removed a can file with bad license. I created a new tarball and gave the instructions.
Upstream told me the program itself is GPLv2.

> * Sources: OK
> - Source0 URL ok
> - spectool -g canorus.spec works
> - sources matches upstream - md5sum:
> 2dc201fec21d781d0add487c5a9ed35b  canorus_0.7svn.R1163_source.tar.bz2
> - even if the URL for the nightly builds is linked directly from canorus'
> homepage it is a little bit strange to use plain IP addresses here - let's hope
> that upstream does an official release soon and the URL can be changed again to
> something like this:
> http://prdownload.berlios.de/canorus/canorus_0.7.R1002_source.tar.bz2
> 

Yes, I will turn to regular release tarballs when the software turns more stable.

> 
> * Locales handling: TODO
> The package contains language files in a non-gettext format (*.qm files). 
> Is it necessary to add them also via the %lang(xx) tag?
> 

Yes, we do this with qt applications. For instance, I was asked in the past explicitly to mark the .qm files as %lang(xx) for qjackctl and qsynth


> 
> * code vs. content: TODO
> - see above (license issues)
> - package contains example midi files, sheets of music, etc. (3.5 MB)
> - according to the guidelines examples in general are not considered as
> content, but if they e.g. contain notes from music which is still under
> copyright I assume it would not be permissable...
> 

See above

> 
> * functional test: TODO
> - program segfaults when it is closed
> - program segfaults when opening any of the musicxml examples
> 

Yes, the last time I talked to them on IRC upstream knew about the issue. But I didn't contact them in the past 3-4 weeks (I was too busy) so I don't know what is the current situation. Upstream had told me before that canorus is still alpha quality, for the time being. But they did a good job for a start and I see it worth packaging.


Spec URL: http://oget.fedorapeople.org/review/canorus.spec
SRPM URL: http://oget.fedorapeople.org/review/canorus-0.7-3.R1177.20090804svn.fc11.src.rpm

koji rawhide build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1624776
Comment 18 Orcan Ogetbil 2009-09-04 13:41:58 EDT
I fixed the compilation flags issue:

Spec URL: http://oget.fedorapeople.org/review/canorus.spec
SRPM URL:
http://oget.fedorapeople.org/review/canorus-0.7-4.R1177.20090904svn.fc11.src.rpm
Comment 19 Christian Krause 2009-09-19 15:30:06 EDT
I've reviewed the latest package:

(In reply to comment #17)
> Yay! Finally! I'm very sorry for the delay
> 
> (In reply to comment #13)
> > 
> > * naming: TODO
> > - name matches upstream
> > - spec file name matches package name
> > - snapshot release tag (assuming it is a post-release snapshot) should contain
> > the date (the svnrev can be appended, but the date is required)
> > ( according to
> > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages )
> > 
> 
> Added

Ok.

> > * License: TODO
> > - the package contains sources under the GPLv2, too:
> > src/import/pmidi/except.c (GPLv2 as published), most likely this means
> > that the complete package must be released as GPLv2
> > - the source package (and the built binary package) contain lots of examples
> > and so it is necessary to check their legal status - in the worst case they
> > must not only be stripped out from the binary but also from the sources - do
> > you have any information whether they are distributable?
> > - license file packaged: if the final package would be GPLv2, then we should
> > not package GPLv1
> > 
> 
> I removed the midi and the xml files which have unclear licenses. Also removed
> a can file with bad license. I created a new tarball and gave the instructions.
> Upstream told me the program itself is GPLv2.

Ok. Very minor glitch which will not hold the review: The comments about re-packaging the tarball are not consistent with respect to the SVN revision. The line with "wget" refers to release R1174. ;-)

> > * Locales handling: TODO
> > The package contains language files in a non-gettext format (*.qm files). 
> > Is it necessary to add them also via the %lang(xx) tag?
> > 
> 
> Yes, we do this with qt applications. For instance, I was asked in the past
> explicitly to mark the .qm files as %lang(xx) for qjackctl and qsynth

Ok.

(In reply to comment #18)
> I fixed the compilation flags issue:

Ok.

> Spec URL: http://oget.fedorapeople.org/review/canorus.spec
> SRPM URL:
> http://oget.fedorapeople.org/review/canorus-0.7-4.R1177.20090904svn.fc11.src.rpm  

All reported issues were fixed. I've done again a very small functional test and the main functions of the program seems to be working reasonable well.

-> APPROVED
Comment 20 Orcan Ogetbil 2009-09-20 17:55:33 EDT
Thanks for the review. I'll correct the repack instructions in the next update.

New Package CVS Request
=======================
Package Name: canorus
Short Description: Music Score Editor
Owners: oget
Branches: F-10 F-11 F-12
InitialCC:
Comment 21 Jason Tibbitts 2009-09-21 21:49:22 EDT
CVS done.
Comment 22 Fedora Update System 2009-09-22 21:43:27 EDT
canorus-0.7-4.R1177.20090904svn.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/canorus-0.7-4.R1177.20090904svn.fc11
Comment 23 Fedora Update System 2009-09-22 21:44:30 EDT
canorus-0.7-4.R1177.20090904svn.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/canorus-0.7-4.R1177.20090904svn.fc10
Comment 24 Fedora Update System 2009-09-25 16:14:15 EDT
canorus-0.7-4.R1177.20090904svn.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update canorus'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-9951
Comment 25 Fedora Update System 2009-09-25 16:14:31 EDT
canorus-0.7-4.R1177.20090904svn.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update canorus'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-9953
Comment 26 Fedora Update System 2009-10-08 23:41:31 EDT
canorus-0.7-4.R1177.20090904svn.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2009-10-08 23:43:24 EDT
canorus-0.7-4.R1177.20090904svn.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.