Bug 292371 - Review Request: ladspa-caps-plugins - The C* Audio Plugin Suite
Review Request: ladspa-caps-plugins - The C* Audio Plugin Suite
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-16 04:52 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-02 16:04:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kwizart: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-09-16 04:52:53 EDT
Spec URL: <spec info here>
SRPM URL: <srpm info here>
Description: <description here>
Comment 1 Hans de Goede 2007-09-16 04:55:45 EDT
Ugh, that was not supposed to happen, here is the correct review request:

Spec URL: http://people.atrpms.net/~hdegoede/ladspa-caps-plugins.spec
SRPM URL: http://people.atrpms.net/~hdegoede/ladspa-caps-plugins-0.4.2-1.fc8.src.rpm
Description:
caps, the C* Audio Plugin Suite, is a collection of refined LADSPA
units including instrument amplifier emulation, stomp-box classics,
versatile 'virtual analog' oscillators, fractal oscillation, reverb,
equalization and others.
Comment 2 Fernando Lopez-Lezcano 2007-09-20 13:26:32 EDT
By default the new version compiles with:

  -O2 -ffast-math -funroll-loops -Wall -fPIC -DPIC

So the extra flags are no longer needed. 
What I have in %install now is:

----
%install
%{__rm} -rf %{buildroot}
# install x86_64 in the right place
%{__perl} -p -i -e "s|/lib/|/%{_lib}/|g" Makefile
%{__make} PREFIX=%{buildroot}%{_prefix} install
# make sure plugin shared library is executable
%{__chmod} +x %{buildroot}%{_libdir}/ladspa/caps.so
----

Seems to work fine. The perl one liner could be a patch or whatever, without it
plugins go into /usr/lib/ladspa in x86_64. 
Comment 3 Hans de Goede 2007-09-20 14:18:21 EDT
(In reply to comment #2)
> By default the new version compiles with:
> 
>   -O2 -ffast-math -funroll-loops -Wall -fPIC -DPIC
> 
> So the extra flags are no longer needed. 
> What I have in %install now is:
> 

Erm, explain? Passing RPM_OPT_FLAGS is always needed to enable things like
FORTIFY_SOURCE and stack-smashing protection, which are not only usefull for
security reasons, but also for catching normal bugs. Normally RPM_OPT_FLAGS
should be the only optimalisation affecting flags, but when upstream uses
-ffast-math, that may be added.

> ----
> %install
> %{__rm} -rf %{buildroot}
> # install x86_64 in the right place
> %{__perl} -p -i -e "s|/lib/|/%{_lib}/|g" Makefile
> %{__make} PREFIX=%{buildroot}%{_prefix} install
> # make sure plugin shared library is executable
> %{__chmod} +x %{buildroot}%{_libdir}/ladspa/caps.so
> ----
> 

My main development machine is a 64 bits machine, and there the spec as
initially posted works fine, did you try the posted SRPM?
Comment 4 Fernando Lopez-Lezcano 2007-09-20 17:43:16 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > By default the new version compiles with:
> > 
> >   -O2 -ffast-math -funroll-loops -Wall -fPIC -DPIC
> > 
> > So the extra flags are no longer needed. 
> > What I have in %install now is:
> 
> Erm, explain? Passing RPM_OPT_FLAGS is always needed to enable things like
> FORTIFY_SOURCE and stack-smashing protection, which are not only usefull for
> security reasons, but also for catching normal bugs. Normally RPM_OPT_FLAGS
> should be the only optimalisation affecting flags, but when upstream uses
> -ffast-math, that may be added.

Ah, ok, sorry. 
Still, '-funroll-loops' is in upstream but missing in my build logs (unless _I_
am missing something yet again - maybe that compiler option no longer makes sense?)

> > ----
> > %install
> > %{__rm} -rf %{buildroot}
> > # install x86_64 in the right place
> > %{__perl} -p -i -e "s|/lib/|/%{_lib}/|g" Makefile
> > %{__make} PREFIX=%{buildroot}%{_prefix} install
> > # make sure plugin shared library is executable
> > %{__chmod} +x %{buildroot}%{_libdir}/ladspa/caps.so
> > ----
> 
> My main development machine is a 64 bits machine, and there the spec as
> initially posted works fine, did you try the posted SRPM?

Nope, of course not :-(
I does work fine. Again, sorry for the noise. 
Comment 5 Hans de Goede 2007-09-21 02:26:45 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > Erm, explain? Passing RPM_OPT_FLAGS is always needed to enable things like
> > FORTIFY_SOURCE and stack-smashing protection, which are not only usefull for
> > security reasons, but also for catching normal bugs. Normally RPM_OPT_FLAGS
> > should be the only optimalisation affecting flags, but when upstream uses
> > -ffast-math, that may be added.
> 
> Ah, ok, sorry. 
> Still, '-funroll-loops' is in upstream but missing in my build logs (unless _I_
> am missing something yet again

Above kept for context of question below.

> - maybe that compiler option no longer makes sense?)
 

I believe it doesn't, some upstreams tend to use all kind of funky
optimalization options, and usually those are a bad idea. For one gcc as in
Fedora gets extensively tested with $RPM_OPT_FLAGS, setting other options might
trigger unknown bugs. In this specific case -funroll-loops may have helped when
upstream last did some benchmarking, but on modern CPU's smaller code is usually
faster as modern CPU's are bandwidth limited and smaller code uses less
bandwitdh (and cache, and instruction trace cache, and brench predictor slots
and ...) -funroll-loops causes code to get bigger and thus usually slower. gcc
with -O2 will already unroll some loops, but only when this is beneficially (for
example a small loop with 2 iterations might actually have less code when
unrolled). I think -O2 includes -funroll-loops, but not -funroll-all-loops, but
I'm not sure. Either way usually its a good idea to not out smart the gcc
developers and just use -O2. -ffast-math is an exception here as -ffast-math is
never enabled by default as it breaks certain aspects of the C-standard causing
(very small) precision loss in floating point operations, but since upstream is
using it I guess the slightly lower lever of precision is acceptable.
Comment 6 Nicolas Chauvet (kwizart) 2007-10-02 05:43:53 EDT
I can take the review...

I will consider the cflags discution solved...

(mock testing in progress on fc6 and devel x86_64 )
Comment 7 Nicolas Chauvet (kwizart) 2007-10-02 07:05:07 EDT
- Why ladspa-caps-plugins and not ladspa-caps ?
 Well, i don't meant to change that as others plugins are already built with
this scheme...
- Might be interesting to patch Makefile to have install -p -m 644 caps.rdf

* Cflags are OK
* directory ownership are OK
* build in mock (tested for fc6 and devel on x86_64)

Seems good
Comment 8 Hans de Goede 2007-10-02 08:30:34 EDT
(In reply to comment #7)
> - Why ladspa-caps-plugins and not ladspa-caps ?
>  Well, i don't meant to change that as others plugins are already built with
> this scheme...

Well because it contains plugin for ladspa, notice how CAPS stands for the
C* Audio *Plugin* Suite, so thats why we have plugins in the name, as for the
order of ladspa-foo-plugins, well thats how things have been done in CCRMA for a
while, so lets keep it like that.

> * Cflags are OK
> * directory ownership are OK
> * build in mock (tested for fc6 and devel on x86_64)
> 
> Seems good

Good, so can it be approved then, or do you want a new version? And if you want
a new version what do you want changed?

Comment 9 Nicolas Chauvet (kwizart) 2007-10-02 08:55:28 EDT
Well, that's a minor problem with timestamp:
- Might be interesting to patch Makefile to have install -p -m 644 caps.rdf ...

I expect if someone do a presto repository it can help to have timestamp not
been changed at install time for data...

But anyway as this is minor i can approve it now if you patch it before tagging...
Comment 10 Hans de Goede 2007-10-02 08:58:32 EDT
OK, I will fix the timestamp before import.
Comment 11 Nicolas Chauvet (kwizart) 2007-10-02 09:03:21 EDT
-----------

This package (ladspa-caps-plugins) is APPROVED by me

----------
Comment 12 Hans de Goede 2007-10-02 09:49:35 EDT
Thanks!

New Package CVS Request
=======================
Package Name:      ladspa-caps-plugins
Short Description: The C* Audio Plugin Suite
Owners:            jwrdegoede nando
Branches:          F-7 devel
InitialCC:         <empty>
Cvsextras Commits: Yes
Comment 13 Kevin Fenzi 2007-10-02 12:10:33 EDT
cvs done.
Comment 14 Hans de Goede 2007-10-02 16:04:39 EDT
Imported and build, closing.

Note You need to log in before you can comment on or make changes to this bug.