Bug 541978 - Review Request: pulseaudio-equalizer - A 15 Bands Equalizer for PulseAudio
Summary: Review Request: pulseaudio-equalizer - A 15 Bands Equalizer for PulseAudio
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 541738 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-11-27 18:52 UTC by Hicham HAOUARI
Modified: 2010-01-07 21:54 UTC (History)
4 users (show)

Fixed In Version: 2.4-1.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-07 21:54:09 UTC
Type: ---
Embargoed:
lkundrak: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Hicham HAOUARI 2009-11-27 18:52:17 UTC
Spec URL: http://hicham.fedorapeople.org/pulseaudio-equalizer.spec
SRPM URL: http://hicham.fedorapeople.org/pulseaudio-equalizer-2.4-1.fc12.src.rpm
Description: PulseAudio Equalizer

rpmlint output : none

Comment 1 Felix Kaechele 2009-11-30 08:03:05 UTC
*** Bug 541738 has been marked as a duplicate of this bug. ***

Comment 2 Howard Ning 2009-12-12 01:57:26 UTC
Seems you have 2 sources one is .tar.gz and one is .deb. The official site only provides the .deb one. The description is too simple in my opinion. The equalizer is a good idea but I have a bad feeling packaging from a deb.

Comment 3 Hicham HAOUARI 2009-12-12 02:05:06 UTC
This is just a temporary solution. I mailed the author and he told me that he is gonna move the project to launchpad soon.

As for the two files, the tar.gz is a dummy file that is overwritten during the build. I had two solutions, this one, or write a custom %setup. I opted for the one because things go smoothly after making the tar.gz from the deb.

Comment 4 Lubomir Rintel 2009-12-20 09:11:20 UTC
Hicham, this looks useful and you've done a good job packing this, unfortunately upstream knows dick about properly maintaining a project. I'm still willing to review this, but am hesitant to approve it until upstream's problems that make it impossible to do a proper review are gone.

Legend:
  * good
  - bad
  ॐ wtf

* Package is named properly
* Version correct and matches upstream
ॐ Can't verify if sources match, could not download it
   (see below)
* License is ok
ॐ License is present in source tarball and installed as %doc
   (see below)
- Spec file is clean and legible
  In fact it mostly is, just have to get rid of that pre-%setup voodoo in %prep
- Does not build
  Enough said above. What you do in %prep is not only unnecessary, but also totally sick. In my case it BUILD/usr in the package and deleted it :) Please, never do anything outside your %{buildsubdir}. Unpack the other sources under it and if necessary create additional top-level directory with %setup -c.
* Filelist sane
* Requires/provides sane

Action points:

Please ask upstream for anonymously accessible address of tarball which they use to produce their .debs, eventually publish them unless they already do.

Please ensure that tarball contains a license file.

I believe a sane source tarball would make the %prep shit unnecessary.

Comment 5 Lubomir Rintel 2009-12-20 09:19:20 UTC
Two more issues:

1.) This file name is not nice: /usr/bin/pulseaudio-equalizer.sh
How about stripping .sh?

2.) According to FHS, proper place for user commands is /usr/bin.
Exec=/usr/share/pulseaudio-equalizer/pulseaudio-equalizer.py
How about making /usr/bin/pulseaudio-equalizer-gui wrapper or something like that?

Comment 6 Hicham HAOUARI 2009-12-20 22:09:02 UTC
I just mailed the author and he told me that he won't put the sources in his launchpad account until mid january.

And by the way, there seems to be a better equalizer which doesn't depend on ladspa in upstream pulseaudio : http://pulseaudio.org/wiki/SystemEqualizer, but this one has a Qt interface only.

I am wondering if i should go with this package if upstream do have better solutions.

Comment 7 Lubomir Rintel 2009-12-21 07:51:49 UTC
(In reply to comment #6)
> I just mailed the author and he told me that he won't put the sources in his
> launchpad account until mid january.
...
> I am wondering if i should go with this package if upstream do have better
> solutions.  

Don't let that discourage you. Until upstream comes to senses, you can get at lease get rid of that gross %prep hack. You should probably use the .deb file as the only source, use %setup -c -T and unpack it with ar afterwards. (And of course address the other issues, which are more-or-less trivial).

Feel free to ask for help should you need any!

Comment 8 Hicham HAOUARI 2009-12-21 16:40:14 UTC
Ok, I have updated the spec file.

Comment 9 Lubomir Rintel 2009-12-24 09:50:16 UTC
(In reply to comment #8)
> Ok, I have updated the spec file.  

Looks better (probably the best that can be done).

APPROVED

Just stumbled upon one tiny issue; probably a typo, please correct it upon CVS import:

1.) You seem to be missing a period here (sh vs .sh):
sed -i s/%{name}sh/%{name}/ usr/share/%{name}/%{name}.py

Comment 10 Hicham HAOUARI 2009-12-24 12:32:43 UTC
Fixed the typo, thanks

Comment 11 Hicham HAOUARI 2009-12-24 12:39:14 UTC
New Package CVS Request
=======================
Package Name: pulseaudio-equalizer
Short Description: A 15 Bands Equalizer for PulseAudio
Owners: hicham
Branches: F-12
InitialCC: hicham

Comment 12 Kevin Fenzi 2009-12-29 03:18:17 UTC
This is icky... 

Can you at least: 

a) provide a full url to the .deb in Source0? 
or
b) provide a comment on exactly where this deb can be obtained? 
or
c) wait until upstream releases some kind of sane release?

Comment 13 Hicham HAOUARI 2009-12-29 22:37:51 UTC
in response to comment #12 , i choosed option b)

the spec and srpm are updated

and sorry kevin for causing so much trouble

Comment 14 Kevin Fenzi 2010-01-02 20:12:34 UTC
No problem. Please do continue to pressure upstream into setting up a proper repository/etc. 

cvs done.

Comment 15 Fedora Update System 2010-01-03 00:06:44 UTC
pulseaudio-equalizer-2.4-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/pulseaudio-equalizer-2.4-1.fc12

Comment 16 Fedora Update System 2010-01-07 21:54:04 UTC
pulseaudio-equalizer-2.4-1.fc12 has been pushed to the Fedora 12 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.