Bug 226035 - Merge Review: libogg
Merge Review: libogg
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:26 EST by Nobody's working on this, feel free to take it
Modified: 2008-03-24 20:42 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-24 20:42:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:26:05 EST
Fedora Merge Review: libogg

http://cvs.fedora.redhat.com/viewcvs/devel/libogg/
Initial Owner: besfahbo@redhat.com
Comment 1 Brian Pepple 2007-02-03 13:42:56 EST
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}'?
Comment 2 Pace Willisson 2007-02-03 17:30:30 EST
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
Comment 3 Matthias Clasen 2007-02-03 19:59:11 EST
I think the ruling is still out on forcing everybody who installs an automake
macro to require automake.
Comment 4 Matthias Clasen 2007-02-08 09:34:43 EST
* Thu Feb  8 2007 Matthias Clasen <mclasen@redhat.com> - 2:1.1.3-3
- Package review cleanups
- Don't ship a static library

Comment 5 Patrice Dumas 2007-02-08 10:20:22 EST
(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.
Comment 6 Ralf Corsepius 2007-02-08 10:23:18 EST
(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.
Comment 7 Patrice Dumas 2007-02-08 10:36:56 EST
(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.
Comment 8 Jason Tibbitts 2007-02-08 14:51:55 EST
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.
Comment 9 Matthias Clasen 2007-06-17 01:13:03 EDT
libogg-devel-1.1.3-4.fc8 requires automake
Comment 10 Hans de Goede 2007-11-14 10:30:29 EST
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.
Comment 11 Tom "spot" Callaway 2007-11-14 10:40:09 EST
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.
Comment 12 Hans de Goede 2007-11-14 14:09:42 EST
(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.
Comment 13 Tom "spot" Callaway 2007-11-14 16:12:01 EST
This is approved, thanks for the hard work!

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