Bug 455953 (rakarrack) - Review Request: rakarrack - Audio effects processing rack for guitar
Summary: Review Request: rakarrack - Audio effects processing rack for guitar
Keywords:
Status: CLOSED NEXTRELEASE
Alias: rakarrack
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-19 07:57 UTC by David Timms
Modified: 2008-12-21 08:44 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-10-20 12:29:13 UTC
Type: ---
Embargoed:
oget.fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description David Timms 2008-07-19 07:57:46 UTC
Spec URL: http://members.iinet.net.au/~timmsy/rakarrack/rakarrack.spec
SRPM URL: 
http://members.iinet.net.au/~timmsy/rakarrack/rakarrack-0.2.0-0.1.fc9.src.rpm

Description: Rakarrack is a basic rack of effects for guitar. 10 effects. Two EQ
(multiband and parametric), distortion, overdrive, echo, chorus,
flanger, phaser, compression and reverb. Real time processing. JACK
support. Online tuner. Bank & Preset managment.

Most of the effects are based on the magnificent work done by Paul
Nasca Octavian in ZynAddSubFX synthesizer. The compressor is based on
ArtsCompressor of Matthias Kretzer & Stefen Westerfeld. The tuner was
adapted from tuneit, a tuner in text mode created by Mario Lang. Paul
Nasca is our hero and a continuous inspiration

I have one package in fedora [glglobe], and one package under review [pyvnc2swf].

The spec is based on a combination of suse-packman and fedora-ccrma packages.

$ rpmlint RPMS/i386/rakarrac*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
[davidt@davidtdesktop redhat]$ rpmlint SRPMS/rakarrack-0.2.0-0.1.fc9.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I haven't actually got my guitar out yet to check operation is OK.

Comment 1 David Timms 2008-07-28 22:25:19 UTC
Oops. I noticed that I am ending up placing two desktop items in the Audio Video
submenu, one without an icon. I think I noticed newer rakarrak includes a
.desktop internally, need to check it out.

Comment 2 David Timms 2008-08-01 14:13:16 UTC
(In reply to comment #1)
> Oops. I noticed that I am ending up placing two desktop items in the Audio Video
> submenu, one without an icon.

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

- exclude the upstream .desktop file, in favour of fedora one.

2. A request was made from planetccrma users to add two additional categories to
the .desktop file:
# desktop file categories
BASE="X-Fedora Application AudioVideo"
XTRA="X-Digital_Processing X-Jack"

These put the audio processing tools into a separate single menu. I am happy to
do that if the packaging guidelines don't block me from doing so {which my
reading suggests not to make new categories}. However, there are a lot of
related audio tools that audio experimenters will find useful to be accessible
from the same menu. Any thoughts on whether this is allowed ?

Comment 3 Orcan Ogetbil 2008-10-04 08:32:53 UTC
The package is pretty good shape. Here are my notes
-------------------------------------------------------------------------
Please fix or comment about items listed with [?] and [x].

[.]: good/pass
[x]: bad/fail
[o]: ugly... just kidding, this means N/A
[?]: questions/comments

[.] MUST: rpmlint must be run on every package. The output should be posted in the review.
rpmlint is silent
[.] MUST: The package must be named according to the Package Naming Guidelines .
[.] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption on Package Naming Guidelines .
[.] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[?] 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).
[.] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
[.] MUST: The spec file must be written in American English.
[.] MUST: The spec file for the package MUST be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora is not the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/).
[.] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. 
e836896fe76aae6aba447e44cdceba4f1c3d422feaf43857bb8e3e041378d297  rakarrack-0.2.0.tar.gz
[.] MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.
[o] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. 
[.] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[o] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[o] MUST: Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
[o] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.
[.] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. Refer to the Guidelines for examples.
[.] MUST: A package must not contain any duplicate files in the %files listing.
[.] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
[.] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ).
[.] MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines .
[.] MUST: The package must contain code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines .
[o] MUST: Large documentation files should go in a -doc subpackage.
[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.
[.] MUST: ScriptletSnippets are used properly.
[o] MUST: Header files must be in a -devel package.
[o] MUST: Static libraries must be in a -static package.
[o] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
[o] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[o] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
[o] MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. 
[.] MUST: Packages must not own files or directories already owned by other packages. 
[.] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). See Prepping BuildRoot For %install for details.
[.] MUST: All filenames in rpm packages must be valid UTF-8. 
[.] MUST: Latest version is packaged.
[?] 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}  ?
[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
[.] MUST: final provides and requires are sane
[.] MUST: package builds in mock
[.] MUST: package installs and removes properly.
-------------------------------------------------------------------------
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
-------------------------------------------------------------------------
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
-------------------------------------------------------------------------
http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
implies that you can add/remove categories. Did I miss something? 
-------------------------------------------------------------------------
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.

Comment 4 David Timms 2008-10-05 12:05:31 UTC
(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

Comment 5 David Timms 2008-10-05 12:20:18 UTC
ps: Is this a pre-review ? I can't seem to find your user name in FAS.

Comment 6 Orcan Ogetbil 2008-10-06 01:18:39 UTC
(In reply to comment #4)
> (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.
> 
Thanks!

-------------------------------------------------------------------------
> > [?] 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.
> 
That's fine, as long as you turn to the standard at the end of the day.

-------------------------------------------------------------------------
> > [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 !
> 
Uh oh, sorry my bad. I missed that the %configure macro takes care of the CFLAGS and CXXFLAGS. About -f{no}exceptions : If two opposite flags are passed, the latter is picked up. So you are fine in this case. You can keep it the way you had in the 0.1 spec file. 

In the future make sure that nothing overrides the opt flags. (Always the last ones are picked up!)

-------------------------------------------------------------------------

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

Thanks! But I need to correct myself here. 
   %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/$dim/apps
   %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_$dim.png \
        %{buildroot}%{_datadir}/icons/hicolor/$dim/apps/rakarrack.png

would be better. None of the existing icons I saw in hicolors/$dim/apps/ are named as application.$dim.png . They are all named as application.png . Let's keep the thing as they are.
-------------------------------------------------------------------------
> > 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.
> 
You can overwrite the existing .desktop file with %Source1 after "make install" so there won't be an ambiguity. I think the final .desktop file in the RPM should be named rakarrack.desktop (not rakarrack-fedora.desktop , this would be unusual!).
-------------------------------------------------------------------------
A few other important things:

In the spec file, avoid using %macros directly inside comments and changelogs. e.g. this is WRONG:
   # This comment concerns %build and %{__make}
The macros in this example will expand out when the RPM is built. If want to indicate a macro in a comment or in the changelog do it the CORRECT way:
   # This comment concerns %%build and %%{__make}
-------------------------------------------------------------------------
I recommend using the %{name} macro instead of hardcoding the name of the application into the spec file; i.e. in this case replacing the occurences of rakarrack * with %{name} is the preferred way. Same thing applies for the %{version} macro.

* except in the beginning: "Name: rakarrack" where you are actually defining %{name}
-------------------------------------------------------------------------
I love planetccrma. I used that repo a lot when I was recording stuff. Maybe we should convince them to merge with rpmfusion.
Btw, my FAS username is oget. I am reviewing the package. No worries.

Comment 7 Orcan Ogetbil 2008-10-06 09:50:44 UTC
Btw, you can also use "sed" to replace items in the original .desktop file and "echo some_new_items >> file" to add items to the .desktop file; so that you won't have to add a new source file. 

Don't forget to change the icon name in the .desktop file to rakarrack.png

Comment 8 David Timms 2008-10-12 02:53:49 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> Thanks! But I need to correct myself here. 
>    %{__mkdir} -p %{buildroot}%{_datadir}/icons/hicolor/$dim/apps
>    %{__mv} %{buildroot}%{_datadir}/pixmaps/icono_rakarrack_$dim.png \
>         %{buildroot}%{_datadir}/icons/hicolor/$dim/apps/rakarrack.png
> 
> would be better. None of the existing icons I saw in hicolors/$dim/apps/ are
> named as application.$dim.png . They are all named as application.png . Let's
> keep the thing as they are.
Yes, reading freedesktop .desktop spec says that preferably the icon should be just name, and the app can then choose either an svg, or most appropriate size png. Giving this a go.

> You can overwrite the existing .desktop file with %Source1 after "make install"
> so there won't be an ambiguity. I think the final .desktop file in the RPM
> should be named rakarrack.desktop (not rakarrack-fedora.desktop , this would be
> unusual!).
Actually, the typical names from what I can see are fedora-rakarrack.desktop. This seems to be based on the vendor tag. The vendor item has been removed from the desktop-file-install command.

> In the spec file, avoid using %macros directly inside comments and changelogs.
> e.g. this is WRONG:
I'll keep an eye out for my attempted quick comments ;-)

> I recommend using the %{name} macro instead of hardcoding the name of the
> application into the spec file; i.e. in this case replacing the occurrences of
> rakarrack * with %{name} is the preferred way. Same thing applies for the
> %{version} macro.
Done.

OK. Finally got the sed and echo mods to the desktop file to work correctly:
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-3.fc9.src.rpm

* Sun Oct 12 2008 David Timms <iinet.net.au @ dtimms> - 0.2.0-3
- don't exclude the original .desktop file

* Mon Oct 06 2008 David Timms <iinet.net.au @ dtimms> - 0.2.0-2
- mod icon names to be appname.png to fit with fallback desktop spec
- replace rakarrack with name macro
- mod .desktop via sed and cat, rather than additional Source file
- add .desktop extra categories using the desktop-file-install utils

Comment 9 David Timms 2008-10-12 03:17:04 UTC
(In reply to comment #3)
> http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
> implies that you can add/remove categories. Did I miss something? 

The original PlanetCCRMA spec included:

# desktop file categories
BASE="X-Fedora Application AudioVideo"
XTRA="X-Digital_Processing X-Jack"

%{__mkdir} -p %{buildroot}%{_datadir}/applications
desktop-file-install --vendor fedora \
  --dir %{buildroot}%{_datadir}/applications \
  `for c in ${BASE} ${XTRA} ; do echo "--add-category $c " ; done` \
  %{SOURCE1}

see also:
https://www.redhat.com/archives/fedora-packaging/2008-October/msg00096.html

From what you mention, it is OK to add extra categories; I was under the impression that only the categories mentioned on freedesktop are considered OK ?

Comment 10 David Timms 2008-10-12 04:11:02 UTC
License has been confirmed as GPLv2.
https://sourceforge.net/forum/forum.php?thread_id=2323002&forum_id=778861

Comment 11 Orcan Ogetbil 2008-10-12 06:17:21 UTC
Thanks. Still there are some issues
*: needs fixed .  
?: doesn't need fixed but my preference is different than yours. Take it or don't.

* rakarrack.desktop -> %{name}.desktop

? %{name} is a basic rack of effects for guitar... -> Rakarrack is a basic rack of effects for guitar...

because this is a Description. Unless the application's name is strictly beginning with a non-capital letter, I would refer to it capital-lettered in sections like Summary or Description.

* This line needs to be in the %prep section:
   %configure --docdir=%{_docdir}/%{name}-%{version} --htmldir=%{_docdir}/%{name}-%{version}

* These lines need to be in the %prep section too:
   %{__sed} -i 's/Icon=icono_rakarrack_128x128/Icon=rakarrack/' %{buildroot}%{_datadir}/applications/rakarrack.desktop
   %{__sed} -i 's/Guitar Effects Processor/Real-time audio effects processing rack for guitar/' %{buildroot}%{_datadir}/applications/rakarrack.desktop
   echo "GenericName=Digital audio effects processor" >> %{buildroot}%{_datadir}/applications/rakarrack.desktop
   echo "Version=1.0" >> %{buildroot}%{_datadir}/applications/rakarrack.desktop

You may need to change the "%{buildroot}%{_datadir}/applications/" to "data/"

Basically, the %build section is for building/compiling, %install section is to install the software into %{buildroot}. Everything else that can be done before coming to these sections must be done in %prep.

*  %doc AUTHORS README
   %{_datadir}/doc/%{name}/COPYING
   %{_datadir}/doc/%{name}/html

This has problems. Now there are two document directories created: /usr/share/doc/rakarrack /usr/share/doc/rakarrack-0.2.0
You only need one document directory. COPYING and html needs to go into %doc (which is /usr/share/doc/rakarrack-0.2.0).
What you have to do is to make the program point onto the correct document directory when you click on the Help->contents button. You may need to hack the code (with some patch) to achieve this or most likely (and hopefully) it will be enough just to pass certain parameters to the configure script. Look at the output of "./configure --help" to see what parameters you may use.

------
So you decided to not add a new category? I'd say that would be convenient for people dealing with audio software. :)

Comment 12 Orcan Ogetbil 2008-10-12 06:23:45 UTC
Actually, the last thing in the previous message that I wrote was:

* This line needs to be in the %prep section:
   %configure --docdir=%{_docdir}/%{name}-%{version}
--htmldir=%{_docdir}/%{name}-%{version}

These may be the right parameters to pass to the configure script. You may need to check it out.

Sorry I forgot to edit my last paragraph accordingly.

(Still %configure needs to go into %prep.)

Comment 13 Orcan Ogetbil 2008-10-12 06:56:04 UTC
OK, this is probably what you need to do (regarding the docs)

   ...
   %prep
   ...
   sed -i 's|HELPDIR="$prefix/share/doc/${PACKAGE}"|HELPDIR="$prefix/share/doc/${PACKAGE}-%{version}"|' configure
   %configure --docdir=%{_docdir}/%{name}-%{version} --htmldir=%{_docdir}/%{name}-%{version}
   ...
   %files
   %doc AUTHORS README html COPYING
   ...

It should be something like this.

Comment 14 David Timms 2008-10-15 13:35:54 UTC
(In reply to comment #11)
> * rakarrack.desktop -> %{name}.desktop
I looked at it and thought, maybe I should assign a variable to store the complete path to .desktop in. Do people normally do things like that ?

Meanwhile I changed to point the spelt out path using %{name}

> ? %{name} is a basic rack of effects for guitar... -> Rakarrack is a basic rack of effects for guitar...
Overzealous search and replace. Fixed.

> * This line needs to be in the %prep section:
>    %configure --docdir=%{_docdir}/%{name}-%{version}
> --htmldir=%{_docdir}/%{name}-%{version}
I'm learning more about make configure and so on, thanks for these pointers.

> * These lines need to be in the %prep section too:
>    %{__sed} -i 's/Icon=icono_rakarrack_128x128/Icon=rakarrack/'
> %{buildroot}%{_datadir}/applications/rakarrack.desktop
>    %{__sed} -i 's/Guitar Effects Processor/Real-time audio effects processing
> rack for guitar/' %{buildroot}%{_datadir}/applications/rakarrack.desktop
>    echo "GenericName=Digital audio effects processor" >>
> %{buildroot}%{_datadir}/applications/rakarrack.desktop
>    echo "Version=1.0" >> %{buildroot}%{_datadir}/applications/rakarrack.desktop
Moved.

> You may need to change the "%{buildroot}%{_datadir}/applications/" to "data/"
Correct, and done.

> Basically, the %build section is for building/compiling, %install section is
> to install the software into %{buildroot}. Everything else that can be done
> before coming to these sections must be done in %prep.
First I've heard of it, but makes complete sense, as long as the build doesn't build the icons / desktop files during make etc on the fly.

> *  %doc AUTHORS README
>    %{_datadir}/doc/%{name}/COPYING
>    %{_datadir}/doc/%{name}/html
> 
> This has problems. Now there are two document directories created:
> /usr/share/doc/rakarrack /usr/share/doc/rakarrack-0.2.0
> You only need one document directory. COPYING and html needs to go into %doc
> (which is /usr/share/doc/rakarrack-0.2.0).
> What you have to do is to make the program point onto the correct document
> directory when you click on the Help->contents button. You may need to hack 
Thanks for the sed hack.
In the end I still had to manually move the files to the doc-versioned dir so that rpm and the app finds these things as expected.
It seems that this is an issue with some part of the source, ie the passed --docdir and --htmldir are not properly used during configure, make and install.
I am not sure what causes the html+COPYING files to end up in a non-versioned doc dir, otherwise I would send a patch upstream.

> So you decided to not add a new category? I'd say that would be convenient for
> people dealing with audio software. :)
I wanted to, and a response from Matthias on f-packaging suggests it is OK, as long as they are in addition to the standard categories, and prepended with X-.
Done.

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-4.fc9.src.rpm

- move non-install commands to setup
- fix configure .ini so that standard help path will be used

Comment 15 Orcan Ogetbil 2008-10-15 19:02:16 UTC
(In reply to comment #14)

*
> (In reply to comment #11)
> > * rakarrack.desktop -> %{name}.desktop
> I looked at it and thought, maybe I should assign a variable to store the
> complete path to .desktop in. Do people normally do things like that ?
> 

I don't think you need that. The way it is is fine.

*
> In the end I still had to manually move the files to the doc-versioned dir so
> that rpm and the app finds these things as expected.
> It seems that this is an issue with some part of the source, ie the passed
> --docdir and --htmldir are not properly used during configure, make and
> install.
> I am not sure what causes the html+COPYING files to end up in a non-versioned
> doc dir, otherwise I would send a patch upstream.

Yes I noticed that. The problem is in the docdir definitions in doc/Makefile.am and doc/Makefile.in files. But sed'ding them is not recommended because then you will need to run autoconf tools which might result in different configure&Makefiles depending on the autoconf version.

I think your workaround (moving the files manually) is just fine until a fix comes from upstream.  

The variable HELPDIR should normally pick up the --htmldir flag you pass to configure. But for some reason they put a hardcoded path for HELPDIR which confused us in the beginning. But make sure you let the upstream know about these problems.

*
> 
> 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-4.fc9.src.rpm
> 
> - move non-install commands to setup
> - fix configure .ini so that standard help path will be used

Great. Last remarks:

* You won't need this line twice. Once (before %configure) is just fine:
   %{__sed} -i 's|HELPDIR="$prefix/share/doc/${PACKAGE}"|HELPDIR="$prefix/share/doc/${PACKAGE}-%{version}"|' configure.in

* When submitting your final version, make sure you remove the commands you commented out that are not relevant. For instance:
   #echo aftt:======

* Careful! :
     #%%{_datadir}/doc/%{name}/COPYING
-->  #%%{_datadir}/doc/%%{name}/COPYING
Well you will need to completely remove this line anyways. But keep in mind that having a macro in a comment with a single % is enough reason to not approve a package. I know the rules are very strict but it is those rules that make fedora consistent. :)

Comment 16 David Timms 2008-10-15 21:41:36 UTC
(In reply to comment #15)
> The variable HELPDIR should normally pick up the --htmldir flag you pass to
> configure. But for some reason they put a hardcoded path for HELPDIR which
> confused us in the beginning. But make sure you let the upstream know about
> these problems.
Will do.

> * You won't need this line twice. Once (before %configure) is just fine:
>    %{__sed} -i
> 's|HELPDIR="$prefix/share/doc/${PACKAGE}"|HELPDIR="$prefix/share/doc/${PACKAGE}-%{version}"|' configure.in

In trying to get html/helpdir to work, I thought the sed changes may have been failing, or overwritten before the make. Unfortunately I left my debug cruft in the spec ;-|, now gone.

> * When submitting your final version, make sure you remove the commands you
> commented out that are not relevant. For instance:
done. it was getting late, and I was keen to have something published so that my progress could be reviewed.

> * Careful! :
>      #%%{_datadir}/doc/%{name}/COPYING
> -->  #%%{_datadir}/doc/%%{name}/COPYING
> Well you will need to completely remove this line anyways. But keep in mind
> that having a macro in a comment with a single % is enough reason to not
> approve a package. I know the rules are very strict but it is those rules that
> make fedora consistent. :)
Did it again :(. Actually, this check could possibly be performed by rpmlint -> fail bad specs. I'll take that up with it's upstream.

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-5.fc9.src.rpm

- del debug cruft left in the spec while trying to solve issues

Comment 17 Orcan Ogetbil 2008-10-16 02:19:08 UTC
Well done. Now it's time to play some guitar :)

---------------------------------------------
This package (rakarrack) is APPROVED by oget.
---------------------------------------------

I recommend you to do some package reviews. 
I learned a lot about packaging while doing reviews.

Comment 18 David Timms 2008-10-16 09:57:03 UTC
New Package CVS Request
=======================
Package Name: rakarrack
Short Description: Audio effects processing rack for guitar
Owners: dtimms
Branches: F-8 F-9 EL-5
InitialCC:

ps. oget: I think the review procedures requires you set the assigned to yourself as reviewer.

Comment 19 Orcan Ogetbil 2008-10-16 10:14:30 UTC
sorry, I missed that :)
done

Comment 20 David Timms 2008-10-16 21:15:37 UTC
And for me to set the cvs flag... doh.

Comment 21 Kevin Fenzi 2008-10-19 22:22:56 UTC
cvs done.

Comment 22 David Timms 2008-10-20 12:29:13 UTC
imported, committed and built on devel, F9, F8, EPEL

Comment 23 Fedora Update System 2008-10-20 21:46:27 UTC
rakarrack-0.2.0-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/rakarrack-0.2.0-5.fc9

Comment 24 Fedora Update System 2008-10-20 21:46:31 UTC
rakarrack-0.2.0-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/rakarrack-0.2.0-5.fc8

Comment 25 Fedora Update System 2008-12-21 08:44:37 UTC
rakarrack-0.2.0-5.fc9 has been pushed to the Fedora 9 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.