Bug 209144 - Review Request: alsa-oss - Userspace OSS emulation
Review Request: alsa-oss - Userspace OSS emulation
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Denis Leroy
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-03 11:44 EDT by Jima
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-06 14:17:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Jima 2006-10-03 11:44:24 EDT
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
Description: 
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!
Comment 1 Denis Leroy 2006-10-05 15:41:24 EDT
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
scriplets.

- 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)

Comment 2 Jima 2006-10-05 16:17:43 EDT
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 <jima@beer.tclug.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:
http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss-1.0.12-1.spec

and uploading new spec/SRPM at:
http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss.spec
http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss-1.0.12-2.fc6.src.rpm

Thanks!
Comment 3 Denis Leroy 2006-10-06 08:23:10 EDT
- 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

Some issues:

- 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.

- 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
%ifarch x86_64.

If you agree to these changes, consider the package APPROVED.
Comment 4 Jima 2006-10-06 09:20:33 EDT
> - 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

Done.

> - 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. 
Updated at:

http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss.spec
http://beer.tclug.org/fedora-extras/alsa-oss/alsa-oss-1.0.12-3.fc6.src.rpm

Let me know whether those changes look satisfactory.

Thanks!
Comment 5 Denis Leroy 2006-10-06 09:29:50 EDT
Looks good. Package is APPROVED.
Comment 6 Rex Dieter 2006-10-06 09:42:40 EDT
Re: comment #4
> There's not a no-static-libs rule

for the record, yeah, there kinda is such a rule:
http://fedoraproject.org/wiki/Packaging/Guidelines
"Exclusion of Static Libraries"
Comment 7 Jima 2006-10-06 09:53:39 EDT
The Package Review Guidelines could probably use some rewording, in that case:

- MUST: Header files or static libraries must be in a -devel package.

(http://fedoraproject.org/wiki/Packaging/ReviewGuidelines)

However, I stand corrected.
Comment 8 Jima 2006-10-06 14:17:07 EDT
Package imported into CVS, built successfully for devel, and FC-5 branch requested.

Closing as NEXTRELEASE, thanks!

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