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@xxxxxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review