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

Mon Oct 6 01:18:41 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

Orcan Ogetbil <orcanbahri at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review?

--- Comment #6 from Orcan Ogetbil <orcanbahri at yahoo.com>  2008-10-05 21:18:39 EDT ---
(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.

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