Bug 292371

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.

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. 

(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?

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

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

I can take the review...

I will consider the cflags discution solved...

(mock testing in progress on fc6 and devel x86_64 )

- 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

(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?

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

OK, I will fix the timestamp before import.

-----------

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

----------

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

cvs done.

Imported and build, closing.