Bug 539837 - Review Request: mppenc - Musepack audio compressor
Summary: Review Request: mppenc - Musepack audio compressor
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: David Timms
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-11-21 09:50 UTC by Michael Schwendt
Modified: 2009-11-24 11:09 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-11-24 11:09:42 UTC
Type: ---
Embargoed:
dtimms: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Michael Schwendt 2009-11-21 09:50:09 UTC
Spec URL: http://mschwendt.fedorapeople.org/mppenc.spec
SRPM URL: http://mschwendt.fedorapeople.org/mppenc-1.16-0.1.fc11.src.rpm
Description: This is a Musepack StreamVersion7 encoder.

Musepack is an audio compression format with a strong emphasis on high
quality. It's not lossless, but it is designed for transparency, so that
you won't be able to hear differences between the original wave file and
the much smaller MPC file. It is based on the MPEG-1 Layer-2 / MP2
algorithms, but has rapidly developed and vastly improved and is now at an
advanced stage in which it contains heavily optimized and patentless code.

[...]

Currently, Fedora includes only libmpcdec (the Musepack SV7 decoding library). The heavily changed "Musepack SV8 libs & tools (r435)" upstream distribution will include libraries and tools for compression and decompression. The new compressor will be called "mpcenc" not "mppenc", so the tools can coexist [for some time].

Comment 1 David Timms 2009-11-21 12:19:25 UTC
rpmlint /home/davidt/dev/packaging/reviews/mppenc/mppenc.spec
/home/davidt/dev/packaging/reviews/mppenc/mppenc.spec: W: no-cleaning-of-buildroot %clean
/home/davidt/dev/packaging/reviews/mppenc/mppenc.spec: W: no-buildroot-tag
/home/davidt/dev/packaging/reviews/mppenc/mppenc.spec: W: no-%clean-section
0 packages and 1 specfiles checked; 0 errors, 3 warnings.

The built packages are rpmlint free.

Comment 2 David Timms 2009-11-21 13:41:18 UTC
Things to look at:
1. Missing following stanza (as indicated by rpmlint).

%clean
rm -rf $RPM_BUILD_ROOT


2. missing buildroot stuff
BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)


3. order of stanzas. It seems that normally Name comes first before summary (and some of the other items based on the order that newspec creates. Is there a good reason to go against suggestions from rpmdev-newspec ?

4. the  Install file mentions ossaudio (bsd only). is that a real requirement ?
(I wouldn't think so, since it shouldn't need to actually use audio output or input.)

5. Install also mentions use of:
'cmake -DCMAKE_INSTALL_PREFIX:=/myPath'
Seems it installs into a fedora happy location anyway. So I do not know if this is needed.

6. patch cmake. Is this to ignore the default build params and ensure Fedora's get used. It might be useful to explain that above the patch definition, but I'm not insisting.

7. license: website says License: GNU LGPL
most src files state: License: GNU LGPL v2.1 or later.
It seems the intent is probably LGPLv2+ which matches a standard Fedora  license designator, that is provided in the spec. Is it worth querying upstream to get some license info in the remaining src files "


OK: named as upstream package.

OK. md5sum in archive inside src.rpm matches upstream:
$ md5sum mppenc-1.16.tar.bz2 mppenc-1.16-0.1.fc11.src/mppenc-1.16.tar.bz2
f1456141283814efcc012cfa15609bc6  mppenc-1.16.tar.bz2
f1456141283814efcc012cfa15609bc6  mppenc-1.16-0.1.fc11.src/mppenc-1.16.tar.bz2

OK. rpmbuild -ba succeeds in creating src, bin and bin-debuginfo rpms.

OK. app works as expected to create a mpc file from wav input. The created mpc is playable with audacious-plugins. on i686

OK. the remaining must items pass, or are not relevant to this package. Most of the should items are also not relevant.

Comment 3 Michael Schwendt 2009-11-21 14:26:50 UTC
Thanks for the quick review!

As of an RPM update in Fedora 10 and later,
 
* %buildroot is defined by default,
* BuildRoot definition in the spec file is ignored,
* %buildroot tree is removed automatically at beginning of %install,
* a default %clean section is provided by rpmbuild.

This package actually is my first public one to omit all these items as they are no longer necessary.

See "rpmbuild --clean --rebuild mppenc-1.16-0.1.fc11.src.rpm"

> Is there a good reason to go against suggestions from rpmdev-newspec ?

Personal taste. The order in rpmdev-newspec was completely arbitrary and based on personal taste, too.

> ossaudio

mpp.h says: 
| on some systems you also must link the libossaudio library,
| so maybe you also must edit the Makefile

However, with Fedora there is no dependency on anything like that. No extra header inclusion either. The code path, which evaluates the USE_OSS_AUDIO preprocessor variable, doesn't do anything special. The code reuses a header and definitions from the mpc decoder mppdec.

> 5. Install also mentions use of:
> 'cmake -DCMAKE_INSTALL_PREFIX:=/myPath'
> Seems it installs into a fedora happy location anyway.

See "rpm --eval %cmake".

> 6. patch cmake. Is this to ignore the default build params and
> ensure Fedora's get used. It might be useful to explain that
> above the patch definition,

Good suggestion, as I normally do that unless a patch is really obvious.

> 7. license:

A mail to upstream is on its way.

Comment 5 David Timms 2009-11-22 03:19:01 UTC
(In reply to comment #3)
> Thanks for the quick review!
No probs.

> As of an RPM update in Fedora 10 and later,
> 
> * %buildroot is defined by default,
> * BuildRoot definition in the spec file is ignored,
> * %buildroot tree is removed automatically at beginning of %install,
> * a default %clean section is provided by rpmbuild.
> 
> This package actually is my first public one to omit all these items as they
> are no longer necessary.
OK, that explains why rpmbuild actually succeeds, and the package works.

Closest info I found was in bug #455387, but is there a specific reference in wiki / lists that we can point to ?

Thanks for explanations / info about the other items.

I notice the -0.1, -0.2 package release numbers. It was pointed out to me that these don't meet the packaging standards (because it isn't a beta/prerelease package; I assume you'll import and commit with version -1 ?

I notice there is a lot of warnings during the build on i386/f12, but AFAIK we don't expect packagers to resolve those, so I consider this package:

Approved.

Comment 6 Michael Schwendt 2009-11-22 21:17:35 UTC
> buildroot

The Wiki is partially up-to-date, at least here in the blue boxes:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

It isn't editable, though, so one cannot easily edit the section about %clean.

> compilation warnings

Yes, seen them too. It's unlikely for this project to see a new release, as the Musepack Project continues with the SV8 suite.

> versioning

Yeah, the 0.X scheme is only used during review, to avoid that %release jumps up quickly, and to allow for the first official build, which will start at 1.

It's entirely harmless to use this scheme while it would worse if a pre-release did not use it. ;)

[...]

New Package CVS Request
=======================
Package Name: mppenc
Short Description: Musepack SV7 audio file encoder
Owners: mschwendt
Branches: F-11 F-12
InitialCC:

Comment 7 Kevin Fenzi 2009-11-24 02:07:30 UTC
cvs done.


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