Bug 416461 - Review Request: xmms-pulse - XMMS output plugin for the PulseAudio sound server.
Review Request: xmms-pulse - XMMS output plugin for the PulseAudio sound se...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
: 446441 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-08 06:05 EST by Henry Kroll
Modified: 2008-06-20 15:15 EDT (History)
8 users (show)

See Also:
Fixed In Version: 0.9.4-5.fc9.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-20 15:15:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Henry Kroll 2007-12-08 06:05:13 EST
Spec URL: http://thenerdshow.com/rpm/xmms-pulse.spec
SRPM URL: http://thenerdshow.com/rpm/xmms-pulse-0.9.4-1.fc8.src.rpm
Description: xmms-pulse is an XMMS output plugin for the PulseAudio sound server.
Comment 1 Henry Kroll 2007-12-08 06:14:29 EST
grrr I accidentally hit enter and it won't let me change the title.
Comment 2 Rajeesh 2008-01-10 06:45:28 EST
I'm doing an unofficial review of your package:

1. License 'GPL' is obsolete. Please refer http://fedoraproject.org/wiki/Licensing
2. Group 'Sound and Video' is non standard
3. Please build the SRPM as root, it complains 'user hellork' doesn't exists
Comment 3 Rajeesh 2008-01-10 07:24:04 EST
Here's another one - Build failure with "Checking for xmms-config...no"

I guess you need to add xmms-devel to the 'BuildRequires'
Comment 4 manuel wolfshant 2008-01-10 08:58:07 EST
The user which builds the srpm has no importance at all. mock builds as the user
running it. Using root for rpmbuild-ing is strongly discouraged.
Comment 5 Jason Tibbitts 2008-01-27 20:08:52 EST
Henry, any response?  Also, is this your first package?  I don't see you in the
account system.  If it is your first package, you'll need a sponsor; see
http://fedoraproject.org/wiki/PackageMaintainers/Join and
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
Comment 6 Mamoru TASAKA 2008-02-06 03:58:51 EST
Setting NEEDINFO.
Comment 7 Mamoru TASAKA 2008-02-14 06:31:59 EST
ping again?
Comment 8 Henry Kroll 2008-02-14 20:20:01 EST
Yes? I have several submissions but I am unaware of my sponsor status. If
somebody would like to sponsor me that would be cool.

This is more of a feature request but since I'm using it I thought I'd share
what I got. Sorry about the missing buildrequires. The Fedora packager guide
made no mention of yum deplist.
Comment 9 Mamoru TASAKA 2008-02-17 11:43:49 EST
What I mean by "ping?" is that I want you to update your spec file/srpm
followed by comment 2. If you update your spec/srpm, sponsor members 
(including Jason and me) may want to review your spec/srpm.
Comment 10 Henry Kroll 2008-02-17 16:47:20 EST
Updated 2-14-08

see http://thenerdshow.com/rpm for directory listing of latest work in progress
Comment 11 Mamoru TASAKA 2008-02-26 11:34:16 EST
Well, I cannot see your srpm are updated as your latest srpm
has "xmms-pulse-0.9.4-1.fc8".

Please change the version of your srpm each time you modify your
srpm to avoid confusion. Also, please write the URL of spec/srpm
from which we can download them directly by "wget -N" (for example)
on this bug.
Comment 12 Michael Schwendt 2008-04-13 05:23:40 EDT
> %configure --disable-lynx \
>    --prefix=%{_prefix} \
>    --libdir=%{_libdir} \
>    --mandir=%{_mandir} \

The last three args are the default.
See "rpm --eval %configure".

> %dir %{_libdir}/xmms/Output/libxmms-pulse.la

Surely not a directory. It's a libtool archive file.
Comment 13 Henry Kroll 2008-04-13 20:04:44 EDT
Oops! I posted the wrong spec file. It also had the wrong source package,
license and group! Here is the correct one, now revision 2 to avoid confusion:

http://thenerdshow.com/rpm/xmms-pulse.spec
http://thenerdshow.com/rpm/xmms-pulse-0.9.4-2.fc8.src.rpm
Comment 14 Mamoru TASAKA 2008-04-14 14:59:09 EDT
Well, I have not checked this in detail, however looks good at a
quick glance, will check later.

By the way, as this is a NEEDSPONSOR ticket:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 15 Mamoru TASAKA 2008-04-14 15:09:16 EDT
For 0.9.4-2:

* SourceURL
  - Source0 must be given with full URL:
    http://fedoraproject.org/wiki/Packaging/SourceURL
  - Also I suggest to use %{name}-%{version}.tar.gz. With this style
    you won't have to modify SourceURL when new version is released.

* Cleaning directory at %install
  - Please fix the first line in %install.
Comment 16 Henry Kroll 2008-04-14 18:04:47 EDT
%changelog
* Mon Apr 14 2008 Henry Kroll <nospam[AT]thenerdshow.com>
- 0.9.4-3 clean install, include full source0: path

http://thenerdshow.com/rpm/xmms-pulse.spec
http://thenerdshow.com/rpm/xmms-pulse-0.9.4-3.fc8.src.rpm
Comment 17 Mamoru TASAKA 2008-04-16 12:56:40 EDT
A misc issue
- BR: xmms-libs is redundant as BR: xmms-devel pulls this.

And I will wait for your pre-review or another review request.
Comment 18 Mamoru TASAKA 2008-04-29 11:09:53 EDT
ping?
Comment 19 Henry Kroll 2008-04-30 01:43:51 EDT
I've been busy reviewing Fedora 9 at the moment and preparing to share my
experience on my web site. I did try out some of the packages in the bugzilla
and I even left some comments on a few of them. To answer your question, I don't
think I've done a suitable pre-review yet. However, many of the rpms here are
already corrected or uncorrectable in their current form, binary blobs, license
restrictions, etc.
Comment 20 Mamoru TASAKA 2008-05-14 12:44:18 EDT
*** Bug 446441 has been marked as a duplicate of this bug. ***
Comment 21 Robin Norwood 2008-05-14 14:36:49 EDT
Oops - For reference, here's the spec file I made when I filed the duplicate
request:

http://people.redhat.com/rnorwood/rpms/xmms-pulse.spec
Comment 22 Henry Kroll 2008-05-15 19:00:36 EDT
I like your spec file, except I don't have xmms-config on my system and mock
doesn't seem to pull it in either so there is probably another BuildRequires to
use that. For some reason xmms-devel wasn't pulling in xmms-libs in fedora 8,
but perhaps that problem is fixed now in f9... todo: test. I do not know why I
need libX* or why I don't have these. I believe the --disable-static and libtool
cleanup are necessary for Fedora. Correct me if I'm wrong. I do not remember why
I have --disable-lynx in my spec file. Should we leave lynx as a dependency or
take it out?

I guess these are all the informal pre-review feedback/questions I have. I'm
sure I'll come up with more once I hit save...
Comment 23 Robin Norwood 2008-05-16 10:13:14 EDT
o xmms-config comes from xmms-devel, so I think we're ok, there.

o Also, xmms-devel does indeed require xmms-libs:
$ rpm -q --whatrequires xmms-libs
xmms-devel-1.2.10-38.fc9.i386

o I think you're right about the libtool cleanup business.  I get around that by
installing just the files we care about with %{__install} instead of make
install - I suspect using 'make install' and cleaning up like you're doing is
the preferred method.

o lynx is used to generate the docs (lynx --dump), so I have a README.html and a
style.css - not really a big deal at all though if you want to leave those out.
 I see that I forgot to include the ChangeLog in mine.


Henry,

I'd recommend that you take the xmms_outputdir macro from my spec file and use
it in yours, since I think it's nicer than "%{_libdir}/xmms/Output/", but I
don't think any of the other differences matter.
Comment 24 Mamoru TASAKA 2008-05-29 13:56:35 EDT
What is the status of this bug?
Comment 25 Henry Kroll 2008-05-29 16:37:29 EDT
OK I combined hopefully what is the best of both into one spec file:

http://thenerdshow.com/rpm/xmms-pulse.spec
http://thenerdshow.com/rpm/xmms-pulse-0.9.4-4.fc8.src.rpm

Been awefully busy. Sponsor needed. To expedite this, can someone with
sponsorship take over this bug? Xmms is a popular package and I don't want to
hold up the works.
Comment 26 Mamoru TASAKA 2008-06-04 13:13:29 EDT
Henry, if you want I will once close this bug. 
Comment 27 Robin Norwood 2008-06-04 13:34:35 EDT
Or, since Henry is busy, you can switch over to my version:

http://people.redhat.com/rnorwood/rpms/xmms-pulse.spec
http://people.redhat.com/rnorwood/rpms/xmms-pulse-0.9.4-5.fc10.src.rpm

I've bumped the release to be higher than Henry's latest.
Comment 28 Mamoru TASAKA 2008-06-04 14:54:23 EDT
Well, for 0.9.4-5:

* Comments for libtool
----------------------------------------------------------
# On Fedora x86_64 some libtool archives get created even with --disable-static
----------------------------------------------------------
  - Usually --disable-static is for not creating static archives and
    it is usual that libtool .la files are installed even with
    --disable-static ...
    And on some softwares these libtool files are really needed
    (e.g. ImageMagick)

Other things are okay.
-----------------------------------------------------------
         This package (xmms-pulse) is APPROVED by me
----------------------------------------------------------

Removing NEEDSPONSOR.
Comment 29 Mamoru TASAKA 2008-06-12 01:51:05 EDT
Robin, ping?
Comment 30 Robin Norwood 2008-06-12 10:53:57 EDT
New Package CVS Request
=======================
Package Name: xmms-pulse
Short Description: XMMS output plugin for the PulseAudio sound server
Owners: rnorwood
Branches: F-9
InitialCC:
Cvsextras Commits: Yes
Comment 31 Kevin Fenzi 2008-06-12 12:03:15 EDT
cvs done.
Comment 32 Mamoru TASAKA 2008-06-15 07:47:27 EDT
Please rebuild this package also on F-9 and submit a push request on
bodhi.
Comment 33 Fedora Update System 2008-06-16 12:18:15 EDT
xmms-pulse-0.9.4-5.fc9.1 has been submitted as an update for Fedora 9
Comment 34 Fedora Update System 2008-06-17 23:14:43 EDT
xmms-pulse-0.9.4-5.fc9.1 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xmms-pulse'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-5418
Comment 35 Mamoru TASAKA 2008-06-17 23:19:21 EDT
Closing.
Comment 36 Fedora Update System 2008-06-20 15:15:01 EDT
xmms-pulse-0.9.4-5.fc9.1 has been pushed to the Fedora 9 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.