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!
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)
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.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!
- 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.
> - 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!
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: http://fedoraproject.org/wiki/Packaging/Guidelines "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. (http://fedoraproject.org/wiki/Packaging/ReviewGuidelines) However, I stand corrected.
Package imported into CVS, built successfully for devel, and FC-5 branch requested. Closing as NEXTRELEASE, thanks!