Bug 173054 - Review Request: wavpack - completely open audiocodec
Review Request: wavpack - completely open audiocodec
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
David Lawrence
http://www.wavpack.com/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-11-13 04:01 EST by Peter Lemenkov
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-15 16:38:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Peter Lemenkov 2005-11-13 04:01:42 EST
Spec Name or Url: http://paula.comtv.ru/wavpack.spec
SRPM Name or Url: http://paula.comtv.ru/wavpack-4.3-1.src.rpm

Description: WavPack is a completely open audio compression format providing lossless, high-quality lossy, and a unique hybrid compression mode. Although the 
technology is loosely based on previous versions of WavPack, the new version 4 format has been designed from the ground up to offer unparalleled performance and functionality.
Comment 1 Matt Domsch 2005-11-14 17:43:33 EST
This isn't a formal review, as I can't do that yet, and I'm not 100% 
comfortable doing so, and this appears to be your first package in Fedora, and 
I can't sponsor you.  So this is practice for both of us. :-)

!! building on FC4 x86_64, rpmlint yields 
$  rpmlint RPMS/x86_64/wavpack-4.3-1.x86_64.rpm
E: wavpack binary-or-shlib-defines-rpath /usr/bin/wavpack ['/usr/lib64']
E: wavpack binary-or-shlib-defines-rpath /usr/bin/wvunpack ['/usr/lib64']

$  rpmlint RPMS/x86_64/wavpack-devel-4.3-1.x86_64.rpm
E: wavpack-devel only-non-binary-in-usr-lib

this one appears to be because of the inclusion of the .pc file.

* named per PackageNamingGuidelines
* spec file name matches
* PackagingGuidelines appear to be met
* open source license (BSD)
* license in %doc
* spec in english
* spec file legible
* sources match upstream package (md5sum)
* builds in mock for FC4 x86_64
* no exceptional BuildRequires
* no spec file locales issues
* %pre/%post calling ldconfig properly
* not relocatable
* no directory ownership problems
* no duplicate files
* %files have %defaddr
* package has %clean that contains rm -rf buildroot
* consistent use of macros
* no docs subpackage necessary
* header files in -devel, no static libs
* pkgconfig .pc file in -devel
* library file .so in -devel
* -devel Requires fully versioned base package
* .la files removed
* no desktop file needed, command line only
* license in %doc
* no translations provided
* builds in mock for FC4 x86_64 at least

Comment 2 Michael Schwendt 2005-12-25 11:47:25 EST
* On the pedantic side:
  It is commonly considered bad taste to mention the software name
  in the Summary line. Keep the summary short and include relevant
  keywords. That's enough. More details fit into the package description.

  Probably also  s/wavpack/WavPack/gi  since that is how they
  spell it online.

* pkgconfig template file wavpack.pc.in contains hardcoded libdir,
  which most likely breaks on multilib platforms if installed like
  that. Needs a patch which does  libdir=@libdir@  instead of 
  libdir=${prefix}/lib  and provided that libdir will be defined and
  substituted by the used autotools framework.

* pkgconfig file Cflags line is questionable. Adding a standard path
  for headers to the search list is dangerous. Also, are WavPack
  API users expected to do #include <wavpack/wavpack.h> or
  #include <wavpack.h>? In case of the latter, the pkgconfig file
  is wrong.

  Same for Libs line. -L${libdir} disturbes library location search list
  because libwavpack.so is installed into a standard location.
Comment 3 Peter Lemenkov 2006-01-07 10:06:03 EST
>   It is commonly considered bad taste to mention the software name
>   in the Summary line. Keep the summary short and include relevant
>   keywords. That's enough. More details fit into the package description.

Done.

>   Probably also  s/wavpack/WavPack/gi  since that is how they
>   spell it online.

Done. Except the RPM-name.

> * pkgconfig template file wavpack.pc.in contains hardcoded libdir,
>   which most likely breaks on multilib platforms if installed like
>   that. Needs a patch which does  libdir=@libdir@  instead of 
>   libdir=${prefix}/lib  and provided that libdir will be defined and
>   substituted by the used autotools framework.
> * pkgconfig file Cflags line is questionable. Adding a standard path
>   for headers to the search list is dangerous. Also, are WavPack
>   API users expected to do #include <wavpack/wavpack.h> or
>   #include <wavpack.h>? In case of the latter, the pkgconfig file
>   is wrong.
>   Same for Libs line. -L${libdir} disturbes library location search list
>   because libwavpack.so is installed into a standard location.

Done.

I also updated spec-file due to mainstream version change (4.3 -> 4.31).

Spec Name or Url: http://lemenkov.newmail.ru/SPECS/wavpack.spec
SRPM Name or Url: http://lemenkov.newmail.ru/SRPMS/wavpack-4.31-1.src.rpm
Comment 4 Michael Schwendt 2006-01-12 07:10:13 EST
* spec file looks good
* patch visited
* upstream locations verified
* binary package contents look good
* test WAV file packed/unpacked (i386)

APPROVED

* Caution: 4.3 => 4.31 : 4.4 would be seen as older than 4.31, since 4 < 31 =>
you would need to choose version=4.40 if next release were 4.4

* Debian package contains contributed manual pages and mentions problems on some
archs where "char" is unsigned by default (may need investigation, since
compiler gives several related warnings),
http://packages.debian.org/unstable/sound/wavpack

Consider giving it some testing in FE development first prior to
publishing it for FC4 and older.
Comment 5 Peter Lemenkov 2006-01-12 16:37:57 EST
> * Caution: 4.3 => 4.31 : 4.4 would be seen as older than 4.31, since 4 < 31 =>
> you would need to choose version=4.40 if next release were 4.4

Maybe it would be beter to choose version == 4.3.1 for this package?

> Consider giving it some testing in FE development first prior to
> publishing it for FC4 and older.

Ok.
Comment 6 Michael Schwendt 2006-01-13 16:53:45 EST
To most users, 4.40 looks newer than 4.4.0 while 4.3.1 looks older
than 4.31. ;-)  And you still want to stay close to upstream versioning
scheme.   (the fun starts when you feel the need to bump %{epoch})
Comment 7 Peter Lemenkov 2007-09-26 15:00:18 EDT
Package Change Request
======================
Package Name: wavpack
New Branches: EL-4 EL-5
Comment 8 Kevin Fenzi 2007-09-27 12:33:34 EDT
cvs done.

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