Bug 498136 - Review Request: gst-mixer - advanced mixer for GNOME (old gnome-volume-control)
Review Request: gst-mixer - advanced mixer for GNOME (old gnome-volume-control)
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andreas Thienemann
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2009-04-28 22:22 EDT by Adam Williamson
Modified: 2009-05-10 18:39 EDT (History)
15 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2009-05-04 17:35:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Adam Williamson 2009-04-28 22:22:35 EDT
Spec URL: http://adamwill.fedorapeople.org/gst-mixer/gst-mixer.spec
SRPM URL: http://adamwill.fedorapeople.org/gst-mixer/gst-mixer-2.26.0-1.aw_fc11.src.rpm
Description: A full control ALSA mixer application for GNOME. Per FESco meeting of April 24th 2009, for Fedora 11 release.

I previously submitted gnome-alsamixer to satisfy this proposal - https://bugzilla.redhat.com/show_bug.cgi?id=497593 - but Bastien Nocera said the old gnome-volume-control code would be preferable as it is still actively maintained.

This is a package of the old gnome-volume-control code from gnome-media, renamed gst-mixer (the name of the directory it lives in within the gnome-media code).
Comment 1 Itamar Reis Peixoto 2009-04-29 00:47:12 EDT
I have tested and for me are good,

now I can make the volume higher
Comment 2 Tom "spot" Callaway 2009-04-30 16:59:17 EDT
A few things worth noting:

* You really, really, really don't need those macro defines at the top. Half of them aren't used. Just put the versions in the BuildRequires explicitly.
* Fix the patch to apply without fuzz, please? :) Then, drop the default_fuzz macro define.
* The %{gettext_package} macro is fine, but please use %global instead of %define.
* Are you sure --disable-pulseaudio is correct?
Comment 3 Adam Williamson 2009-04-30 19:35:46 EDT
For the first point: I wanted to keep the gst-mixer spec as close as possible to the gnome-media spec, this seemed sensible. So that's where the %define forest comes from. I wouldn't have created it myself. :) If you don't think it's important to have the two specs as similar as possible, I'll happily take them out.

The patch applies without fuzz, I think, that setting just came from the gnome-media spec again. I'll take it out.

Yes, I want --disable-pulseaudio, because we really just want this mixer to talk to ALSA, not Pulse. The *new* mixer is intended to be the mixer for Pulse.

Having said that, it doesn't seem to have any particular effect, because gst-mixer once built still seems able to talk to Pulse - you get a choice in the drop-down in gst-mixer for Pulse mixer as well as ALSA mixer. Sigh.

Updated package coming in a few minutes. I'll wait to hear back on the define forest, but change the other bits.
Comment 4 Adam Williamson 2009-04-30 19:39:16 EDT
OK - spec and .src.rpm updated, same URL.
Comment 5 Tom "spot" Callaway 2009-04-30 21:57:56 EDT
You still really don't need those macros. There is no reason to keep unnecessary cruft in a package, derived from another or not. Also, it is generally good form to bump the release and add an entry to the changelog whenever making a change to the spec file, even during review.
Comment 6 Adam Williamson 2009-05-01 13:10:44 EDT
OK, bumped to -2.aw_fc11 with that change made:

Comment 7 Adam Williamson 2009-05-03 13:30:27 EDT
Comment 8 Kevin Fenzi 2009-05-03 23:16:37 EDT
Andreas: Sorry for hijacking here, but we really need to have this review done. 
I hope you don't mind I take over?

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
3d519bc7d812aed8f6e4288b6d3cdf26  gnome-media-2.26.0.tar.bz2
3d519bc7d812aed8f6e4288b6d3cdf26  gnome-media-2.26.0.tar.bz2.orig
OK - Package needs ExcludeArch
OK - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
OK - final provides and requires are sane.


OK - Should build in mock. 
OK - Should have sane scriptlets. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin


1. Should the 
Requires:       gnome-media
be versioned? 
Or will it cause any issue for this to be out of step with gnome-media?
I suspect not, but though I would mention it. 

2. rpmlint says: 

gst-mixer.src:163: W: macro-in-%changelog defines

Minor issue... change to %%defines in changelog? :) 

gst-mixer.x86_64: W: non-conffile-in-etc /etc/gconf/schemas/gst-mixer.schemas

Can be ignored. 

I don't see any blockers here, so this package is APPROVED. 

Fix the %%define and address if gnome-media requires should be versioned before import?
Comment 9 Adam Williamson 2009-05-04 13:56:50 EDT
I can't really see any big problems that would be caused by not versioning the gnome-media requires, but let me know if I'm missing anything. I'm just about to send a revised -2 package with the changelog changed (it seems a bit recursive to bump to -3 and add a changelog entry to say I changed the changelog...:>)
Comment 10 Adam Williamson 2009-05-04 13:57:41 EDT
OK, it's up now. Same URL as the previous -2 package.
Comment 11 Kevin Fenzi 2009-05-04 14:04:55 EDT
Looks dandy. Feel free to request cvs.
Comment 12 Adam Williamson 2009-05-04 16:29:16 EDT
New Package CVS Request
Package Name: gst-mixer
Short Description: legacy mixer for GNOME
Owners: adamwill
Branches: F-11
Comment 13 Kevin Fenzi 2009-05-04 16:36:26 EDT
cvs done.
Comment 14 Adam Williamson 2009-05-04 17:35:35 EDT
ok, review is complete, rawhide build is done. thanks very much, kevin.

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