Bug 188461 - Review Request: xmms-musepack - Mpegplus (mpc) playback plugin for XMMS
Review Request: xmms-musepack - Mpegplus (mpc) playback plugin for XMMS
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-10 05:50 EDT by Matthias Saou
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-02 18:28:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Spec file patch (1.25 KB, patch)
2006-04-12 04:48 EDT, Matthias Saou
no flags Details | Diff

  None (edit)
Description Matthias Saou 2006-04-10 05:50:36 EDT
Spec Name or Url: http://svn.rpmforge.net/svn/trunk/rpms/xmms-musepack/
SRPM Name or Url: http://freshrpms.net/rpm/xmms-musepack
Description:
X MultiMedia System input plugin to play mpegplus, aka mpc files.

Now that libmpcdec has been included into Extras, this plugin can be too.
Comment 1 Eric Tanguy 2006-04-11 14:24:32 EDT
Review for release 2.fc5:
* RPM name is OK
* This is the latest version
* Builds fine in mock
* rpmlint of xmms-musepack looks OK
* File list of xmms-musepack looks OK

Needs work:
* Source 0 is not available
(http://musepack.origean.net/files/linux/plugins/xmms-musepack-1.2.tar.bz2)
  (wiki: QAChecklist item 2)
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* BuildRequires: gcc-c++ should not be included
  (wiki: PackagingGuidelines#Exceptions)

Maybe you could add %{?dist} in the release. It could be usefull for next fedora
version (ie fc6)
Comment 2 Michael Schwendt 2006-04-11 14:46:06 EDT
> X MultiMedia System input plugin to play mpegplus, aka mpc files.

_Musepack_ aka MPC
Comment 3 Matthias Saou 2006-04-12 04:47:31 EDT
* Source0 will be changed to this (they keep moving files around...) :
  http://files2.musepack.net/linux/plugins/xmms-musepack-%{version}.tar.bz2
* BuildRoot will stay without the useless %(%{__id_u} -n)
* BuildRequires of gcc-c++ will be removed
* "mpegplus" will be changed to "musepack", now that upstream has changed it

I'll also remove the %{prever} stuff since there hasn't been the need to update
to a pre-version in a long time now.
Comment 4 Matthias Saou 2006-04-12 04:48:24 EDT
Created attachment 127652 [details]
Spec file patch
Comment 5 Michael Schwendt 2006-04-12 06:05:13 EDT
>  g++  -O3 -fomit-frame-pointer

means it doesn't accept our beloved optflags.
Comment 6 Matthias Saou 2006-05-03 15:56:13 EDT
Replacing the make line with :

%{__make} %{?_smp_mflags} CXXFLAGS="%{optflags}"

Fixes those CXXFLAGS being set by the configure script. A patch to configure.ac
+ running autoconf seems overkill but would probably be cleanest. Opinions?

Updated 1.2-2.1 spec file available from the same location :
http://svn.rpmforge.net/svn/trunk/rpms/xmms-musepack/
(note that I will remove the 2 first lines and add %{?dist} to the final spec)
Comment 7 Patrice Dumas 2006-07-23 18:03:30 EDT
Some remarks: 
* the buildroot is not the right one
* although the %define xmms_inputdir with xmms-config isn't clearly 
  wrong I think it should not be done that way, since xmms-config
  must then be present at spec file generation time. Simply having
%define xmms_inputdir %{_libdir}/xmms/Input
  or even replacing %xmms_inputdir with %{_libdir}/xmms/Input would
  seem simpler and cleaner to me
* %setup is missing a -q
Comment 8 Michael Schwendt 2006-07-23 19:37:02 EDT
> although the %define xmms_inputdir with xmms-config isn't clearly 
> wrong I think it should not be done that way, since xmms-config
> must then be present at spec file generation time.

No, it need not be present at src.rpm build-time. It must be present
when building the binaries. Also notice the " || echo ..." which
ensures that all this still works when xmms-config is not found.
Comment 9 Patrice Dumas 2006-07-24 09:16:45 EDT
(In reply to comment #8)
> No, it need not be present at src.rpm build-time. It must be present
> when building the binaries.

Of course :/. Why did I made that wrong assumption???
Comment 10 Patrice Dumas 2006-07-24 09:18:11 EDT
(In reply to comment #7)
> * although the %define xmms_inputdir with xmms-config isn't clearly 
>   wrong I think it should not be done that way, 

Forget about that insane comment, but why isn't the package that
provides xmms-config BuildRequired?


Comment 11 Matthias Saou 2006-07-24 09:26:47 EDT
Because xmms-devel is build required already, and it requires xmms in turn. So
xmms-config will be present and used inside the properly set up build environment.
Comment 12 Ville Skyttä 2006-07-24 13:06:55 EDT
(In reply to comment #11)
> Because xmms-devel is build required already, and it requires xmms in turn.

Nope, it requires xmms-libs nowadays, but:

> So xmms-config will be present

Yep, it's in the xmms-devel package.
Comment 13 Patrice Dumas 2006-07-24 17:11:44 EDT
Still not right:
- gcc-c++ should be omitted from buildrequires
- %setup -q
- buildroot is wrong

In the %changelog, the lines are like
* Wed May  3 2006 Matthias Saou <http://freshrpms.net/> 1.2-2.1
the strange thing in it is <http://freshrpms.net/> instead of a 
mail adress. Is it acceptable?


* rpmlint gives
W: xmms-musepack setup-not-quiet
* follow naming and packaging guidelines
* licence is BSD-like, included
* spec legible
* source match upstream
ff7f5f9122d09ad63af9c564046086cf  xmms-musepack-1.2.tar.bz2
* builds and works on devel
* use macros consistently
* don't own any directory
* %files section right
* no missing BuildRequires
* only dlopened library, in the right directory for xmms

If the above issues are solved I would approve that package
Comment 14 Matthias Saou 2006-07-25 04:13:14 EDT
Yes, gcc-c++ will be removed. But as for the others :
- Nothing in the guideline forces to quiet the %setup step.
- BuildRoot is fine, guidelines only hint a "preferred" value. I keep needing to
  repeat over and over that I find it plain silly to execute "id" in there, since
  we're building in a dedicated build system anyway, always as the same user.
This is quite OT for the review, but IMHO both the quiet %setup and the
BuildRoot should be things that should be configured with sane defaults outside
of the spec files, for which repeating in each and every spec file should *not*
be needed...
Comment 15 Ralf Corsepius 2006-07-25 05:32:37 EDT
(In reply to comment #14)

> - BuildRoot is fine, guidelines only hint a "preferred" value.
A fact I consider to be a defect of the guidelines.

> I keep needing to
>   repeat over and over that I find it plain silly to execute "id" in there,
And I will repeat over and over again:
 Your buildroot is broken. It is not multi-user safe.
Comment 16 Matthias Saou 2006-07-25 05:44:39 EDT
My BuildRoot is perfectly multi-user safe! Te only thing a user needs to do is
add a line for %_tmppath to his ~/.rpmmacros. Or use mock/mach, which is what
users should all be doing anyway.
Repeat it over and over again as much as you like, but for me the default
BuildRoot shoudn't be inside the spec file in the first place anyway. It's silly
to have the exact same value in each and every spec file.

Why does this have to happen for each and every review request I make? :-)
Comment 17 Paul Howarth 2006-07-25 06:00:34 EDT
(In reply to comment #16)
> Why does this have to happen for each and every review request I make? :-)

It's also been discussed on fedora-packaging recently:
https://www.redhat.com/archives/fedora-packaging/2006-July/msg00167.html

It's also one of the items to be discussed by the packaging committee:
http://fedoraproject.org/wiki/Packaging/GuidelinesTodo

  "Should a fixed %buildroot be made mandatory? The current FE "SHOULD"
   doesn't consider %arch. Axel's %buildroot is not multi-user capable."

It doesn't seem to have a high priority on the agenda, so it probably needs
someone to drive it forward on the mailing lists and establish a proposal that
can be voted on.
Comment 18 Patrice Dumas 2006-07-25 06:08:19 EDT
(In reply to comment #16)

> BuildRoot shoudn't be inside the spec file in the first place anyway. It's silly
> to have the exact same value in each and every spec file.

As long as it is in the spec file it should be the the same in the fedora
extras spec files, and set to the most relevant value.
Comment 19 Ralf Corsepius 2006-07-25 06:13:45 EDT
(In reply to comment #16)
> My BuildRoot is perfectly multi-user safe! Te only thing a user needs to do is
> add a line for %_tmppath to his ~/.rpmmacros.
This is complete nonsense.

With a similar .rpmmacros you don't ever need to set BuildRoot.


Comment 20 Patrice Dumas 2006-07-25 06:17:31 EDT
(In reply to comment #14)

> - Nothing in the guideline forces to quiet the %setup step.

It generates a rpmlint warning. And there is no reason to omit
it, since the setup output, at least in that case isn't of 
interest.
Comment 21 Ralf Corsepius 2006-07-25 06:19:34 EDT
In reply to comment #16)
> Or use mock/mach, which is what
> users should all be doing anyway.
Forgot to mention: mock isn't multiuser-capable as well:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=198515
Comment 22 Matthias Saou 2006-08-23 11:12:01 EDT
Spec file for 1.2-3 : http://ftp.es6.freshrpms.net/tmp/extras/xmms-musepack.spec

Please feel free to review, it's been a while, and I'd really like this package
to get out of the queue...
Comment 23 Patrice Dumas 2006-08-23 11:17:46 EDT
I am ready to approve, but with the -q to setup. It is not
required by the guidelines, but it generates a rpmlint warning, 
isn't usefull and goes against what is done in most packages.
I may approve it without the -q if you convince me that 
%setup without -q adds anything else than output lines.
Comment 24 Matthias Saou 2006-08-23 11:36:46 EDT
Here we go again :-)

I won't try to convince you about anything, just point out two things :
- Guideline discussions don't belong in package submission entries, please put
the review on hold and start discussing on the maintainers, packaging or extras
list if you feel something should be addressed.
- From my POV, the -q to setup is silly, and should be configurable in the build
system directly, pretty much like defattr, buildroot, clean... but again, this
is something to bring up on a public list, not here.
Comment 25 Rex Dieter 2006-08-23 11:55:53 EDT
Fer cryin out loud, let's just bang this out and be done with it...
Comment 26 Rex Dieter 2006-08-23 12:08:40 EDT
  
* %excluding the .la file is probably not necessary, but not bad to do so 
either.

* Provided SOURCE: url didn't work for me, but this did:
http://files2.musepack.net/linux/plugins/xmms-musepack-1.2.tar.bz2
Comment 27 Rex Dieter 2006-08-23 12:11:10 EDT
upstream source checks out:
ff7f5f9122d09ad63af9c564046086cf  xmms-musepack-1.2.tar.bz2

Everything else is simple, clean...
APPROVED.  (and please fix the Source: URL, before requesting any builds)
Comment 28 Matthias Saou 2006-08-23 12:17:46 EDT
What spec file did you look at? The last link I posted already had the source
you mention in comment #26 and it seems to work. All good to go I guess, thanks :-)
Comment 29 Patrice Dumas 2006-08-23 12:20:19 EDT
Such a felony against the holy -q, assigned traitor! 

This sad day, a battle I lost ;-)
Comment 30 Rex Dieter 2006-08-23 12:23:09 EDT
I must have looked at the spec from the last src.rpm (1.2-2?).  oops.

Patrice, sure, mark me a traiter, I just don't see why reviews should be held up
on semantics and/or maintainer preferences (ie, non MUST items)
Comment 31 Patrice Dumas 2006-08-23 12:34:24 EDT
(In reply to comment #30)
> I must have looked at the spec from the last src.rpm (1.2-2?).  oops.
> 
> Patrice, sure, mark me a traiter, I just don't see why reviews should be held up
> on semantics and/or maintainer preferences (ie, non MUST items)

No problem, I truely approve your felony, since it is a non 
MUST item :-)

But with my high principles on setup -q I couldn't approve 
that evil specfile ;-)  
Comment 32 Kevin Fenzi 2006-09-02 16:56:22 EDT
This package has been imported and built. 
Can this request be closed NEXTRELEASE?
Comment 33 Matthias Saou 2006-09-02 18:28:04 EDT
Strange... I could have sworn I had closed it. Sorry, done now.

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