Red Hat Bugzilla – Bug 209144
Review Request: alsa-oss - Userspace OSS emulation
Last modified: 2007-11-30 17:11:45 EST
Spec URL: http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss.spec
SRPM URL: http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss-1.0.12-1.fc6.src.rpm
This package contains the compatibility library and wrapper script for
running legacy OSS applications through ALSA. Unlike the kernel
driver, this has the advantage of supporting DMIX software mixing.
This package is technically a duplicate of Bug 187706, a stalled review. Please do not mark it as such; the current reviewer has signed off on filing this review bug. Thanks!
Jima, a first list of issues before I do a more formal review:
- tarball is 1.0.12 but Version field is 1.0.11 :-) As you know we want 1.0.12
since we have to match the version of alsa-lib that's in FC6. While fixing that,
you can also remove the prever macro and rc3 release tags, as well as the '-n'
option on the %prep command ?
- Requires: /sbin/ldconfig not necessary when ldconfig called with -p option
- Nit pick: there's a tradition of puting the package version in the patch file,
as well a a short tag to describe it (aka foo-1.0.2-fixthis.patch)
- Can you remove the commented-out autoreconf ? Comments are not welcomed within
- I don't think the CFLAGS= is necessary on the configure line, but you may want
to double check.
- configure needs --disable-static (and *.a removed from %files)
- .la files need to be removed (from %files also)
No offense, Denis, but are you sure you're looking at the right alsa-oss.spec?
The one at the URL above (and from the SRPM) definitely has Version: 1.0.12.
Also notably missing from my spec are the prever macro, rc3 release tag, -n
option on %prep, Requires: /sbin/ldconfig, and CFLAGS= on the configure line.
It should have the following as the last changelog entry:
* Tue Oct 03 2006 Patrick "Jima" Laughton <firstname.lastname@example.org> 1.0.12-1
- Hijacked from stalled review (BZ#187706)
- Bumped to 1.0.12 for devel branch
- Removed Req for /sbin/ldconfig (unnecessary when using -p in scriptlets)
- Added dist tag!
- Made macros slightly more consistent
- Deleted .la files in %%install
I will certainly agree with you on the non-versioned patch and commented-out
autoreconf; both of these were from the previous submission of alsa-oss, and I
didn't bother correcting them. I'm doing so now.
Adding --disable-static to configure; I apologize that I have little experience
with libraries, much less static ones. I'm removing the .a files, but I already
removed the .la files (again...old spec?).
I'm preserving my original spec as:
and uploading new spec/SRPM at:
- rpmlint ok (see below)
- package name ok
- packaging guidelines ok
- licence is GPL, is included in %doc
- source tarball matches upstream
- compiles, builds, and works as expected (tested with firefox+flash)
- dyn libraries handled correctly
- no dir ownership issues
- .so and includes in devel package
- no .la in package
X x86_64 issues (see below)
- builds in mock
- Don't delete libossredir.a: move it to the devel package. This is a wrapper
library specifically meant for static linking (that's why the static linking is
hardcoded by upstream), to make oss redirection more efficient than the
LD_PRELOAD "hack". I think this qualifies as a valid exception to the
- As such, you can remove the rm -fr *.a line
- Add oss-redir/README to the devel package as a %doc
- Missing %defattr in the %files devel section
- x86_64 build suffers from rpath issues. The only fix i could find is to call
'autoreconf -f -i' before configure, and use '--disable-rpath' on the configure
line (and add automake and libtool as BRs). These changes can be protected by an
If you agree to these changes, consider the package APPROVED.
> - Don't delete libossredir.a: move it to the devel package. This is a wrapper
> library specifically meant for static linking (that's why the static linking
> is hardcoded by upstream), to make oss redirection more efficient than the
> LD_PRELOAD "hack". I think this qualifies as a valid exception to the
> no-static-libs rule.
There's not a no-static-libs rule, it just says keep 'em in -devel (which I did
before). Fair enough point, though; fixed.
> - Add oss-redir/README to the devel package as a %doc
> - Missing %defattr in the %files devel section
Ack! Corrected, thanks for noticing.
> - x86_64 build suffers from rpath issues. The only fix i could find is to call
> 'autoreconf -f -i' before configure, and use '--disable-rpath' on the
> configure line (and add automake and libtool as BRs). These changes can be
> protected by an %ifarch x86_64.
As I don't have any x86_64 machines for development/testing purposes, I would
never have caught that. I've made (I think) the appropriate fixes to my spec.
Let me know whether those changes look satisfactory.
Looks good. Package is APPROVED.
Re: comment #4
> There's not a no-static-libs rule
for the record, yeah, there kinda is such a rule:
"Exclusion of Static Libraries"
The Package Review Guidelines could probably use some rewording, in that case:
- MUST: Header files or static libraries must be in a -devel package.
However, I stand corrected.
Package imported into CVS, built successfully for devel, and FC-5 branch requested.
Closing as NEXTRELEASE, thanks!