Bug 188461
Summary: | Review Request: xmms-musepack - Mpegplus (mpc) playback plugin for XMMS | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Matthias Saou <matthias> | ||||
Component: | Package Review | Assignee: | Rex Dieter <rdieter> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | bugs.michael, eric.tanguy, pertusus | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-09-02 22:28:04 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 163779 | ||||||
Attachments: |
|
Description
Matthias Saou
2006-04-10 09:50:36 UTC
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) > X MultiMedia System input plugin to play mpegplus, aka mpc files.
_Musepack_ aka MPC
* 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. Created attachment 127652 [details]
Spec file patch
> g++ -O3 -fomit-frame-pointer
means it doesn't accept our beloved optflags.
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) 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 > 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.
(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??? (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? 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. (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. 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 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... (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. 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? :-) (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. (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. (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. (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. 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 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... 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. 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. Fer cryin out loud, let's just bang this out and be done with it... * %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 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) 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 :-) Such a felony against the holy -q, assigned traitor! This sad day, a battle I lost ;-) 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) (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 ;-) This package has been imported and built. Can this request be closed NEXTRELEASE? Strange... I could have sworn I had closed it. Sorry, done now. |