Bug 527059 - Review Request: earcandy - Sound level manager
Review Request: earcandy - Sound level manager
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-03 15:41 EDT by Lubomir Rintel
Modified: 2009-12-29 03:55 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-29 03:55:42 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lubomir Rintel 2009-10-03 15:41:45 EDT
SPEC: http://v3.sk/~lkundrak/SPECS/earcandy.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/earcandy-0.5.1-1.fc12.src.rpm

Description:

A sound level manager that nicely fades applications in and out based on
their profile and window focus. It is useful if you need to fade out music
or video players during VoIP calls, switch to music player with focus when
more using than one, push sound to USB headsets on plugin, etc. Settings
can be configured via nice and simple UI.
Comment 1 Lubomir Rintel 2009-10-03 15:43:03 EDT
rpmlint complains about various files in python_sitelib having a shebang despite not being executable. Those files are really not meant to be executable and patching away the shebang is not worth since it does not affect functionality at all.

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1726062
Comment 2 Martin Gieseking 2009-10-05 02:29:06 EDT
A couple of comments:

- the file pulseaudio/lib.py is licensed under BSD

- you should replace the shebang of file ear_candy according to https://fedoraproject.org/wiki/Script_Interpreters_%28draft%29


$ rpmlint /var/lib/mock/fedora-11-x86_64/result/earcandy-0.5.1-1.fc11.*
earcandy.noarch: E: explicit-lib-dependency pulseaudio-libs
earcandy.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/ear_candy/window/WindowWatcher.py 0644 /usr/bin/env
earcandy.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/ear_candy/Freedesktop.py 0644 /usr/bin/env
earcandy.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/ear_candy/ear_candy.py 0644 /usr/bin/env
earcandy.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/ear_candy/TrayIcon.py 0644 /usr/bin/env
earcandy.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/ear_candy/__init__.py 0644 /usr/bin/env
2 packages and 0 specfiles checked; 6 errors, 0 warnings.
Comment 3 Martin Gieseking 2009-10-05 03:57:05 EDT
Even if the shebangs don't hurt, I would remove them anyway, e.g. with 

for f in `find $RPM_BUILD_ROOT%{python_sitelib}/ear_candy/ -name '*.py'`; do 
  sed '/^#!\/usr\/bin\/env python/d' $f >$f.new
  touch -r $f $f.new
  mv $f.new $f
done

After doing so, there's still a remaining error. It can be ignored because package pulseaudio-libs is not picked up automatically.

$ rpmlint ../RPMS/noarch/earcandy-0.5.1-1.fc11.noarch.rpm 
earcandy.noarch: E: explicit-lib-dependency pulseaudio-libs
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
Comment 4 Lubomir Rintel 2009-10-05 04:40:28 EDT
(In reply to comment #2)
> A couple of comments:

Thanks for them!

> - the file pulseaudio/lib.py is licensed under BSD

Fixed.

> - you should replace the shebang of file ear_candy according to
> https://fedoraproject.org/wiki/Script_Interpreters_%28draft%29

Fixed.

> Even if the shebangs don't hurt, I would remove them anyway, e.g. with 

Done

> After doing so, there's still a remaining error. It can be ignored because
> package pulseaudio-libs is not picked up automatically.

I do not think so; even if we dlopen libpulse, we still do assumptions about ABI, therefore I changed this to depend on specific ABI version and the rpmlint warning went away.

SPEC: http://v3.sk/~lkundrak/SPECS/earcandy.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/earcandy-0.5.1-2.fc12.src.rpm
Comment 5 Martin Gieseking 2009-10-08 10:45:52 EDT
(In reply to comment #4)
> I do not think so; even if we dlopen libpulse, we still do assumptions about
> ABI, therefore I changed this to depend on specific ABI version and the rpmlint
> warning went away.

Yes, I think you're right.


Could you please upload the SRPM again, e.g. to fedorapoeple.org? The above link doesn't work (404).
Comment 6 Lubomir Rintel 2009-10-17 06:11:02 EDT
SPEC: http://v3.sk/~lkundrak/SPECS/earcandy.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/earcandy-0.5.1-2.fc11.src.rpm

Sorry, I typo-ed the disttag.
Comment 7 Martin Gieseking 2009-10-17 06:51:13 EDT
OK, thanks.

Two further things I noticed that should be addressed:

- you can drop Requires: python because it's added automatically

- the desktop file is added twice:

$ rpmls earcandy-0.5.1-2.fc11.noarch.rpm | fgrep .desktop
-rw-r--r--  /usr/share/applications/earcandy.desktop
-rw-r--r--  /usr/share/earcandy/earcandy.desktop
Comment 8 Lubomir Rintel 2009-10-18 03:12:26 EDT
Thanks you for the comments, the fixes addressing both issues have been applied. New package:

SPEC: http://v3.sk/~lkundrak/SPECS/earcandy.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/earcandy-0.5.1-3.fc12.src.rpm
Comment 9 Hans de Goede 2009-12-21 07:58:58 EST
Reviewing this one, Martin thanks for all the useful comments so far.

Full review done (tarbal matches upstream, license check, spec file readability, rpmlint, etc.). I've only one remark, which is a blocker though.

MUST FIX:
---------
* Requires:       libpulse.so.0(PULSE_0)
  Is wrong for 64 bit systems, which have:
  libpulse.so.0(PULSE_0)(64bit)

The only way I see to fix this, while allowing the package to stay noarch is
using:
Requires: pulseaudio-libs
Comment 10 Lubomir Rintel 2009-12-24 04:56:44 EST
Hans: changed (actually requiring < 1, quetly and probably wrongly assuming that soname's number corresponds to the major version number)

SPEC: http://v3.sk/~lkundrak/SPECS/earcandy.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/earcandy-0.5.1-4.fc12.src.rpm
Comment 11 Hans de Goede 2009-12-24 13:52:02 EST
(In reply to comment #10)
> Hans: changed (actually requiring < 1, quetly and probably wrongly assuming
> that soname's number corresponds to the major version number)
> 

Yes that assumption is most likely wrong, I would advise you to remove the
< 1 in the requires before building, other then that it now looks ok: Approved!
Comment 12 Lubomir Rintel 2009-12-24 15:25:37 EST
(In reply to comment #11)
> (In reply to comment #10)
> > Hans: changed (actually requiring < 1, quetly and probably wrongly assuming
> > that soname's number corresponds to the major version number)
> > 
> 
> Yes that assumption is most likely wrong, I would advise you to remove the
> < 1 in the requires before building

Ok, will do.

New Package CVS Request
=======================
Package Name: earcandy
Short Description: Sound level manager
Owners: lkundrak
Branches: F-11 F-12 EL-5
Comment 13 Kevin Fenzi 2009-12-28 21:57:26 EST
cvs done.
Comment 14 Lubomir Rintel 2009-12-29 03:55:42 EST
Imported and built.

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