Good:

* Package name conforms to the Fedora Naming Guidelines
* Group Tag is from the official list
* All paths begin with macros
* All necessary BuildRequires listed.
* Package builds in Mock.

Must Fix:
* The devel package require should be:
Requires: libogg = %{epoch}:%{version}-%{release}
* Is the static lib necessary?  If so, it should be split out into a
sub-package.  Refer to:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

Minor:
* Doesn't use preferred buildroot:
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* Drop the '.' from the summary to quite rpmlint.
* To clean out the install & clean section, you should probably use 'rm -rf
$RPM_BUILD_ROOT' for consistancy
* Does this package build using 'make %{_smp_mflags}'?

Upstream tar file name is incorrect.  Should be:
http://downloads.xiph.org/releases/ogg/libogg-${version}.tar.gz

libogg-devel needs "Requires: automake" due to installing
/usr/share/aclocal/ogg.m4

I think the ruling is still out on forcing everybody who installs an automake
macro to require automake.

* Thu Feb  8 2007 Matthias Clasen <mclasen> - 2:1.1.3-3
- Package review cleanups
- Don't ship a static library

(In reply to comment #3)
> I think the ruling is still out on forcing everybody who installs an automake
> macro to require automake.

There is no need to require automake, it is also possible to own
/usr/share/aclocal/
I personally would let that choice to the packager.

(In reply to comment #5)
> (In reply to comment #3)
> > I think the ruling is still out on forcing everybody who installs an automake
> > macro to require automake.
> 
> There is no need to require automake, it is also possible to own
> /usr/share/aclocal/
> I personally would let that choice to the packager.
There is no choice but to require automake.

(In reply to comment #6)

> There is no choice but to require automake.

Why? It may be useful, in that people wanting to use the 
macro in their project don't have to install automake by
hand, but automake is not necessarily needed when doing 
development with a library. automake brings in perl, it
is a big dependency.

It's pretty much the same issue as pkgconfig files; you can't own the directory
they go in, so you must have a dependency on the package that does.

libogg-devel-1.1.3-4.fc8 requires automake

To all interested reviewers, I've become a libogg co-maitainer recently and
I would like to push libogg through its merge review. 

(In reply to comment #1)
> Must Fix:
> * The devel package require should be:
> Requires: libogg = %{epoch}:%{version}-%{release}

Already fixed in current rawhide version.

> * Is the static lib necessary?
Already removed in the current rawhide version.

> Minor:
> * Doesn't use preferred buildroot:
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> * Drop the '.' from the summary to quite rpmlint.
> * To clean out the install & clean section, you should probably use 'rm -rf
> $RPM_BUILD_ROOT' for consistancy
All 3 already fixed in the current rawhide version.

> * Does this package build using 'make %{_smp_mflags}'?
Just added it to CVS, should show up in rawhide soon.

(In reply to comment #8)
> It's pretty much the same issue as pkgconfig files; you can't own the directory
> they go in, so you must have a dependency on the package that does.

Requires: automake already present in current rawhide -devel package

---

So all is fixed now, please review and tell me what needs fixing.

From rpmlint:

libogg.src:90: W: macro-in-%changelog doc
libogg.src: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 1)

The only other thing that I see is that you could macro %{name} and %{version}
into the Source tag.

(In reply to comment #11)
> From rpmlint:
> 
> libogg.src:90: W: macro-in-%changelog doc
> libogg.src: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 1)
> 
> The only other thing that I see is that you could macro %{name} and %{version}
> into the Source tag.

Thanks for the review!

All 3 fixed in CVS and on its way to Rawhide.

This is approved, thanks for the hard work!