Bug 904658

For 0.5.1-1

* License
  - Some files are under GPLv3+, and some of them are actually
    used to build the binary:
-----------------------------------------------------
audio/faudiostream/architecture/faust/audio/alsa-dsp.h
audio/faudiostream/architecture/faust/audio/coreaudio-dsp.h
audio/faudiostream/architecture/faust/audio/netjack-dsp.h
audio/faudiostream/architecture/faust/audio/portaudio-dsp.h
audio/faudiostream/architecture/faust/gui/console.h
audio/faudiostream/architecture/faust/gui/faustqt.h
audio/faudiostream/architecture/faust/gui/meta.h
faudiostream/architecture/faust/audio/alsa-dsp.h
faudiostream/architecture/faust/audio/coreaudio-dsp.h
faudiostream/architecture/faust/audio/netjack-dsp.h
faudiostream/architecture/faust/audio/portaudio-dsp.h
faudiostream/architecture/faust/gui/console.h
faudiostream/architecture/faust/gui/faustqt.h
faudiostream/architecture/faust/gui/meta.h
-----------------------------------------------------
    So the license tag should be "GPLv3+" (just GPLv3+
    is okay).

* Compiler flags
  - Fedora specific compilation flags are not used
    correctly. build.log says:
-----------------------------------------------------
    33  Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.ESDXyl
    34  + umask 022
    35  + cd /builddir/build/BUILD
    36  + cd radium_compressor-0.5.1
    37  + make 'OPTS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables' -j5
    38  cd Qt && ./create_source_from_ui.sh `../find_moc_and_uic_paths.sh uic` `../find_moc_and_uic_paths.sh moc` compressor_widget
    39  g++ -DDEBUG -O3 -Wall -msse -mfpmath=sse -DUSE_QT_REQTYPE -DUSE_QT4 -g -I. -ffast-math -IQt Qt/Qt_SliderPainter.cpp `pkg-config --cflags Qt3Support` -c
    40  g++ -DDEBUG -O3 -Wall -msse -mfpmath=sse -DUSE_QT_REQTYPE -DUSE_QT4 -g -I. -ffast-math -IQt main.cpp Qt_SliderPainter.o -Iaudio/faudiostream/architecture/ `pkg-config --libs --cflags Qt3Support` -ljack -o radium_compressor
    41  + exit 0
 -----------------------------------------------------
    Also, "-O3" is used, this should be changed to -O2.
    And, is it safe for this package to use "-ffast-math"?
    -ffast-math does some "optimization" which breaks IEEE rules
    for example, which may result in confusing behavior.

* rpmlint issue
  - incorrect-fsf-address (not a blocker)
    - Some source files contain incorrest FSF address, which causes
      rpmlint complaint on debuginfo rpm. Not a blocker, however it
      is recommend to tell upstream

  - spurious-executable-perm
    - rpmlint complains about some header files in debuginfo rpm for
      having non-proper permission. You can suppress this rpmlint complaint
      by changing the permission of header files to 0644 at %prep.

* Desktop file
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Desktop_files
  - It seems that "radium_compressor" is a GUI program, so
    installing proper desktop file is required (unless there is
    some reason, e.g. this program usually needs proper argument,
    for example)

As far as I can read from http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Optimize-Options.html, -ffast-math should be safe, and benchmarking showed that -ffast-math makes the program run 13% faster.

(I'm the upstream author)

Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning -ftree-vectorize" runs at the same speed as "-O3".

(In reply to comment #3)
> Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least
> on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning
> and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning
> -ftree-vectorize" runs at the same speed as "-O3".

Well, -O2 vs -O3 is rather Fedora packaging (policy) issue. I can accept "-O2 -fpredictive-commoning -ftree-vectorize" instead.

(In reply to comment #3)
> Oh, I also see that -O3 makes the program run twice as fast as -O2 (at least
> on my computer): make benchmark && ./benchmark. It's -fpredictive-commoning
> and -ftree-vectorize that makes the difference. "-O2 -fpredictive-commoning
> -ftree-vectorize" runs at the same speed as "-O3".

Oh, https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Compiler_flags
says 
"Overriding these flags for performance optimizations (for instance, -O3 instead of -O2) is generally discouraged. If you can present benchmarks that show a significant speedup for this particular code, this could be revisited on a case-by-case basis. "

So as you kindly presented benckmarks, I can accept -O3 and -ffast-math.
Still other Fedora specific compilation flags (such as -Wp,-D_FORTIFY_SOURCE=2 and so on) must be honored.

Thanks guys for the throguh investigation of the compiler flags. Really appreciated.

Kjetil: there are some incorrect FSF addresses in your source headers (correct is 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA). You can update this at your disgression

License has been ajusted and file permissions addressed. 

I have added the desktop file and will liase with upstream regarding a suitable icon.

SPEC: http://bsjones.fedorapeople.org/reviews/radium-compressor.spec
SRPM: http://bsjones.fedorapeople.org/reviews/radium-compressor-0.5.1-2.fc18.src.rpm

Some notes:

* Optimization level
  - For changing optimization level, please add some comments on
    your spec file about the discussion on this review request
    (especially the comments / explanation from Kjetil)
    for justification.

! Honoring Fedora specific compilation flags
  - Well, it seems somewhat strange that you have to use sed for Makefile
    and also change "OPTS" variables to honor Fedora specific compilation
    flags, however not a blocker.

-------------------------------------------------------------
   This package (radium-compressor) is APPROVED by mtasaka
-------------------------------------------------------------

(In reply to comment #7)
> Some notes:
> 
> * Optimization level
>   - For changing optimization level, please add some comments on
>     your spec file about the discussion on this review request
>     (especially the comments / explanation from Kjetil)
>     for justification.

Good call - will do.

> 
> ! Honoring Fedora specific compilation flags
>   - Well, it seems somewhat strange that you have to use sed for Makefile
>     and also change "OPTS" variables to honor Fedora specific compilation
>     flags, however not a blocker.
> 

I think this is a feature in the build script. I'll send a patch upstream.

Thanks for the review!

New Package SCM Request
=======================
Package Name: radium-compressor
Short Description: An audio compressor for JACK
Owners: bsjones
Branches: f17 f18

Git done (by process-git-requests).

radium-compressor-0.5.1-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-3.fc17

radium-compressor-0.5.1-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-3.fc18

radium-compressor-0.5.1-3.fc17 has been pushed to the Fedora 17 testing repository.

radium-compressor-0.5.1-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-4.fc17

radium-compressor-0.5.1-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/radium-compressor-0.5.1-4.fc18

Closing.

radium-compressor-0.5.1-4.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

radium-compressor-0.5.1-4.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.