Bug 498136 - Review Request: gst-mixer - advanced mixer for GNOME (old gnome-volume-control)
Summary: Review Request: gst-mixer - advanced mixer for GNOME (old gnome-volume-control)
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Andreas Thienemann
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2009-04-29 02:22 UTC by Adam Williamson
Modified: 2009-05-10 22:39 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2009-05-04 21:35:35 UTC
Type: ---
kevin: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)

Description Adam Williamson 2009-04-29 02:22:35 UTC
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 04:47:12 UTC
I have tested and for me are good,

now I can make the volume higher

Comment 2 Tom "spot" Callaway 2009-04-30 20:59:17 UTC
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 23:35:46 UTC
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 23:39:16 UTC
OK - spec and .src.rpm updated, same URL.

Comment 5 Tom "spot" Callaway 2009-05-01 01:57:56 UTC
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 17:10:44 UTC
OK, bumped to -2.aw_fc11 with that change made:


Comment 7 Adam Williamson 2009-05-03 17:30:27 UTC

Comment 8 Kevin Fenzi 2009-05-04 03:16:37 UTC
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 17:56:50 UTC
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 17:57:41 UTC
OK, it's up now. Same URL as the previous -2 package.

Comment 11 Kevin Fenzi 2009-05-04 18:04:55 UTC
Looks dandy. Feel free to request cvs.

Comment 12 Adam Williamson 2009-05-04 20:29:16 UTC
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 20:36:26 UTC
cvs done.

Comment 14 Adam Williamson 2009-05-04 21:35:35 UTC
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.