Bug 510668

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.

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

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)

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

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

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?

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

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

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.

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

lilypond is fixed in F-10 :) New packages will hit the testing repo soon.

Meanwhile this package still needs a reviewer :)

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

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)

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.

(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

Sorry Christian, I have been really busy lately. I will get back to this in a week or two.

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

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

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

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:

CVS done.

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

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

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

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

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.

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.