Bug 222374 - Review Request: paprefs - Management tool for PulseAudio
Review Request: paprefs - Management tool for PulseAudio
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-01-11 17:07 EST by Eric Moret
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-07-29 23:02:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+

Attachments (Terms of Use)
New spec (1.22 KB, text/plain)
2007-01-23 14:13 EST, Eric Moret
no flags Details

  None (edit)
Description Eric Moret 2007-01-11 17:07:58 EST
Spec URL: ftp://ftp.zouric.com/public/paprefs.spec
SRPM URL: ftp://ftp.zouric.com/public/paprefs-0.9.5-1.src.rpm
Description: PulseAudio Preferences (paprefs) is a simple GTK based configuration dialog for the PulseAudio sound server.

This is my first package and I need a sponsor.
Comment 1 Sindre Pedersen Bjørdal 2007-01-22 20:09:45 EST
This is not a review 

I think calling the package pulseaudio-preferences would be more intuitive. I
wouldn't know to install paprefs to get the pulseaudio preferences. PulseAudio
website even refers to paprefs as pulseaudio volume control on the front page.
According to PackageNamingGuidelines: "If a new package is considered an "addon"
package that enhances or adds a new functionality to an existing Fedora Core or
Fedora Extras package without being useful on its own, its name should reflect
this fact." 

Some other issues to adress: 
* BuildRequires for lynx is commented out, lynx is needed to build
* BuildRequires for desktop-file-utils commented out for some reason

* "--add-category="X-Fedora" --vendor="  is depriciated, you should also add
"--remove-category Application"

* %dir is for owning a dir, but not the contents of that dir. since this package
owns all files in %{_datadir}/paprefs, %dir %{_datadir}/paprefs is not needed.

* Doesn't Require: any of the pulseaudio stack, while paprefs runs without it, I
would imagine it to be pretty useless without pulseaudio actually installed

Good: rpmlint silent, source matches upstream, spec looks good, runs, includes
licencing information, 
Comment 2 Eric Moret 2007-01-23 14:13:54 EST
Created attachment 146339 [details]
New spec

I agree that a name such as pulseaudio-preferences would make more sense. The
reason I named it paprefs was for the sake of consistency. All current gtk
based packages for pulseaudio have the name of their executable: pavumeter,
padevchooser, pavucontrol, paman while all pulseaudio backend package names
start with pulseaudio. Maybe should we rename the gtk based packages to the
more meaningful pulseaudio-whatever.

I am not utterly familiar with desktop-file-install, could you point me to the
page that describe the deprecation of some of those parameters. ie: removing
the "--vendor=" statement resulted in an rpmbuild failure with the below error:

+ desktop-file-install --dir
/var/tmp/paprefs-0.9.5-1-root-emoret/usr/share/applications --remove-category
Must specify the vendor namespace for these files with --vendor

Re: BuildRequires I took the paman spec file as a template to derive the spec
for paprefs. I have addressed all concerns regarding the issues you kindly
outlined in Comment #1 and I think most would also apply to all gtk based
pulseaudio packages (pavumeter, padevchooser, pavucontrol, paman), so you may
want to provide feedback for those other packages as well.
Comment 3 Richard Hughes 2007-02-18 17:42:48 EST
This package is realy useful for configuring pulseaudio without editing config
files. Would be good to get it in for F7.
Comment 4 Eric Moret 2007-02-18 22:46:42 EST
As soon as this package is reviewed and marked APPROVED I'll commit it in cvs 
extras. So far no reviewer has looked at it...
Comment 5 Warren Togami 2007-02-19 20:21:33 EST
Why was this added to the CVS admin requests, even though this package is not
Comment 6 Eric Moret 2007-02-20 00:29:39 EST
The whole process has been a bit confusing to me as well! I initially inquired 
for a sponsorship for the paprefs package but was instead granted it for alsa-
plugins (Bug #222248). However I am not the assignee on that specific bug, so I 
can't do much with it. Anyways, it was my intend to respect the workflow and 
wait until I get this package reviewed before submitting it in CVS. Do you 
think you could spare a few cycles in a review of the paprefs package? If not I 
can just remove that CVS admin request.
Comment 7 Eric Moret 2007-07-24 19:28:03 EDT
Request for review, rebuilt source rpm file:

Comment 8 Jason Tibbitts 2007-07-27 20:52:14 EDT
I really have no idea why this is still sitting around; it's really unfortunate.
 It builds fine and looks clean.  I'm not sure I have any way to test it, but it
runs without crashing and puts up some buttons, and nobody who understands what
it's actually supposed to do has stepped up to review it, so....
Comment 9 Jason Tibbitts 2007-07-27 21:31:16 EDT
* source files match upstream:
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   paprefs = 0.9.5-1.fc8

* %check is not present; no test suite upstream.  Package seems to run fine for 
   me, although I have no audio on the test machine so I've no idea if it works.
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
* GUI app; desktop files exists and looks good to me.

Comment 10 Eric Moret 2007-07-29 01:17:19 EDT
New Package CVS Request
Package Name: paprefs
Short Description: Management GUI tool for PulseAudio
Owners: eric.moret@epita.fr
Branches: FC-6 F-7
Comment 11 Jason Tibbitts 2007-07-29 01:43:28 EDT
CVS done.

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