[Bug 455953] Review Request: rakarrack - Audio effects processing rack for guitar

Sun Oct 5 12:05:32 UTC 2008

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=455953

--- Comment #4 from David Timms <dtimms at iinet.net.au>  2008-10-05 08:05:31 EDT ---
(In reply to comment #3)
> The package is pretty good shape. Here are my notes
Thanks for taking the time to review rakarrack !

> [?] MUST: The License field in the package spec file must match the actual
> license.
> COPYING file says GPLv3 , doc/COPYING file says GPLv2 , {.C} files say GPL
> (version 2) explicitly. I would contact the author and ask what the actual
> license is. If that's not possible I think GPLv2 will be the best option (which
> is what you have already).
As suggested I have posted a forum query on the developers' source forge site:
https://sourceforge.net/forum/forum.php?thread_id=2323002&forum_id=778861
In the meantime, I'll assume GPLv2 since that is what is in the source code.

> [x] MUST: If a package includes something as %doc, it must not affect the
> runtime of the application. To summarize: If it is in %doc, the program must
> run properly if it is not present.
> If you go to the Help-> contents or Help->about->license you will get errors.
> Those need fixed.
Confirmed, and fixed.

> [?] MUST: Dist tag is present and proper.
> Afaik the usual way in Fedora is the usage n%{?dist} where n is the release
> number. Why did you use 0.n%{?dist}  ?
OK. While initially working on the package on my own system, I used the "-0.x"
pre-release scheme. I will properly fit the fedora scheme with the next update.

> [x] MUST: Compiler flags are appropriate. 
> Fedora specific compilation flags are not honored correctly. As the result
> debuginfo rpm is currently not useful. You can check what optflags are used by
>     $ rpm --eval %optflags
> Please see:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
How did you work that out ?
The .debuginfo has a reasonable size, installs OK, contains .c/.h files and
what appears to be the debuginfo. 

The opt flags are included, but some are added twice for example from a
rpmbuild:
-----
if g++ -DHAVE_CONFIG_H -I. -I. -I.     
-O2 -Wall -msse -fno-rtti -pipe -ffunction-sections -fomit-frame-pointer
-Wno-format-y2k -fPIC -fno-exceptions -fno-strict-aliasing 
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -MT FilterParams.o -MD -MP -MF
".deps/FilterParams.Tpo" -c -o FilterParams.o FilterParams.C; \
-----
rpm --eval %optflags
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables

Seems the result would be conflicting options for -f{no}exceptions. I don't
know if the the app will work without that option, and had no success in
avoiding the default CFLAGS. Help wanted !

> -------------------------------------------------------------------------
> Additional comments:
> This code
> 
> # move icons to the proper freedesktop location
> %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/32x32/apps
> %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/64x64/apps
> %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/128x128/apps
> %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_32x32.png \
>         %{buildroot}%{_datadir}/icons/hicolor/32x32/apps/rakarrack.32x32.png
> %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_64x64.png \
>         %{buildroot}%{_datadir}/icons/hicolor/64x64/apps/rakarrack.64x64.png
> %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_128x128.png \
>        
> %{buildroot}%{_datadir}/icons/hicolor/128x128/apps/rakarrack.128x128.png
> 
> will be cleaner if its written as
> for dim in 32x32 64x64 128x128; do
>  %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/$dim/apps
>  %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_$dim.png \
>         %{buildroot}%{_datadir}/icons/hicolor/$dim/apps/rakarrack.$dim.png
> done
OK, I can see that saves a few lines, and it will limit the amount of rework
that might be required if paths needed to be changed etc. Done.

> -------------------------------------------------------------------------
> What is the %exclude for?
> 
> %exclude %{_datadir}/applications/rakarrack.desktop
> %{_datadir}/applications/*desktop
> 
> Can't you just use
> %{_datadir}/applications/*desktop
> or
> %{_datadir}/applications/%{name}.desktop
The source includes a .desktop file, that "make install" puts in the normal
location. The spec generates another with the fedora required bits added, and
prepended with fedora-*. I found that without the %{exclude} I end up with two
Rakarrack items in the menu.

I had tried to remove items from the included .desktop, but didn't seem to be
able to do this with desktop-file-install:
included: rakarrack-0.2.0/data/rakarrack.desktop
-----
[Desktop Entry]
Type=Application
Categories=AudioVideo;Audio;
Exec=rakarrack
Name=Rakarrack
Comment=Guitar Effects Processor
Terminal=false
Icon=icono_rakarrack_128x128
-----
generated:
-----
[Desktop Entry]
Version=1.0
Encoding=UTF-8
Name=Rakarrack
GenericName=Digital audio effects processor
Comment=Real-time audio effects processing rack for guitar
Exec=rakarrack
Icon=rakarrack.64x64.png
Terminal=false
Type=Application
Categories=AudioVideo;Audio;Mixer;

X-Desktop-File-Install-Version=0.15
-----
If there is a better fix example that you know of, let me know.

> -------------------------------------------------------------------------
> http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
> implies that you can add/remove categories. Did I miss something? 
Categories=yes, modifying other items didn't seem possible.

> -------------------------------------------------------------------------
> This is a great application. It took me a while until I put my guitar down and
> finish the review. Thanks for including it in Fedora.
You need to primarily thank Fernando for his CCRMA audio packaging for fedora.
ps. Perhaps you would like to help in Fedorizing more of planetccrma's audio
packages ?

Updated spec: http://members.iinet.net.au/~timmsy/rakarrack/rakarrack.spec
New .src.rpm:
http://members.iinet.net.au/~timmsy/rakarrack/rakarrack-0.2.0-1.fc9.src.rpm

- simplify the icon installation by looping through dimensions
- fix missing ; on category line in desktop file
- don't rename the icons since it confuses desktop-file-install
- fix spelling in description
- fix help menu contents not being displayed due to marking help files doc

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