Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 450975 - Review Request: libcanberra - Portable Sound Event Library
Summary: Review Request: libcanberra - Portable Sound Event Library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-06-12 01:10 UTC by Lennart Poettering
Modified: 2008-06-16 18:06 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-06-16 18:06:22 UTC
mclasen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Lennart Poettering 2008-06-12 01:10:08 UTC
Spec URL: http://0pointer.de/public/libcanberra.spec
SRPM URL: http://0pointer.de/public/libcanberra-0.2-1.fc10.src.rpm
Description: A portable sound event library, implementing the freedesktop.org/XDG sound theming/naming specifications.

Comment 1 Matthias Clasen 2008-06-12 15:04:57 UTC
Builds fine in mock.

rpmlint says: 
[mclasen@localhost ~]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/*.rpm
libcanberra.i386: W: no-documentation

You should include at least
%doc README LGPL


libcanberra.i386: E: no-changelogname-tag

This is odd. Maybe my rpmlint is broken, since there clearly is a %changelog


libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-pulse.so
libcanberra-pulse.so
libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-null.so libcanberra-null.so
libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-alsa.so libcanberra-alsa.so

Are these plugins ? if so, they should not live in /usr/lib


libcanberra-devel.i386: W: non-standard-group Development/C

Should just be Development/Libraries



Comment 2 Matthias Clasen 2008-06-12 15:12:02 UTC
I'll do a formal review in a bit, probably have to leave the office before the
fedora wiki page loads :-)

Comment 3 Matthias Clasen 2008-06-12 15:25:34 UTC
going down the checklist:

package name: ok
spec file name: ok
packaging guidelines: see above for things rpmlint found. beyond that, the 
  devel package needs to require pkg-config because it contains pc files 
  and gtk-doc because it contains stuff in /usr/share/gtk-doc/help
license: ok
license field: ok
license file: must be included
spec file language: ok
spec file legibility: ok
upstream sources: ok
buildable: yes
excludearch: n/a
build deps: ok
locale handling: ok
ldconfig: ok
relocatable: n/a
directory ownership: ok
duplicate files: ok
file permissions: ok
%clean: ok
macro use: consistent
content: permissible
large docs: ok
%doc content: ok
headers: ok
static libs: n/a
pc files: must add Requires: pkgconfig to -devel
shared libs: see above, libcanberra-foo.so should be moved out of /usr/lib
devel deps: ok
libtool archives: ok
gui apps: n/a
file ownership: ok
%install: ok
utf8 filenames: ok








Comment 4 Lennart Poettering 2008-06-12 15:28:50 UTC
> [mclasen@localhost ~]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/*.rpm
> libcanberra.i386: W: no-documentation
> 
> You should include at least
> %doc README LGPL

Fixed.

> libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-pulse.so
> libcanberra-pulse.so
> libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-null.so
libcanberra-null.so
> libcanberra.i386: E: invalid-soname /usr/lib/libcanberra-alsa.so
libcanberra-alsa.so
> 
> Are these plugins ? if so, they should not live in /usr/lib

Yes, they are plugins. Moving plugins to other directores is a pain to do in a
thread-safe manner with the mess that is ltdl, because you need to patch the
search path. I'd prefer leaving it the way it is for now. We can move that
around whenever we want in the future. Also, with the names chosen we shouldn't
be in risk of namespace clashes. And finally, having them in /usr/lib also
allows us to make use of the ld cache. Unless you really insist I'd prefer to
leave it the way it is right now.

> libcanberra-devel.i386: W: non-standard-group Development/C
> 
> Should just be Development/Libraries

Oops, seems to be a mandrivaism left-over from the original spec file I based my
work on.

Fixed, too.

Spec file is now updated.

Comment 5 Lennart Poettering 2008-06-12 15:36:47 UTC
(In reply to comment #3)

> packaging guidelines: see above for things rpmlint found. beyond that, the 
>   devel package needs to require pkg-config because it contains pc files 
>   and gtk-doc because it contains stuff in /usr/share/gtk-doc/help

Fixed.

> license: ok
> license field: ok
> license file: must be included

Fixed. 

> pc files: must add Requires: pkgconfig to -devel

Fixed.

> shared libs: see above, libcanberra-foo.so should be moved out of /usr/lib

See my earlier comments about this.

I uploaded an updated spec/srpm, but didn't bump the revision.


Comment 6 Matthias Clasen 2008-06-13 15:10:32 UTC
> Yes, they are plugins. Moving plugins to other directores is a pain to do in a
> thread-safe manner with the mess that is ltdl, because you need to patch the
> search path.

Sounds like another good reason not to use ltdl, if you ask me. 

> I'd prefer leaving it the way it is for now. We can move that
> around whenever we want in the future. 

I'd be more inclined to agree if this was legacy code. But this is brandnew. If
we don't get it right now, it'll likely broken forever. Don't you think ?


Also, with the names chosen we shouldn't
be in risk of namespace clashes. And finally, having them in /usr/lib also
allows us to make use of the ld cache. Unless you really insist I'd prefer to
leave it the way it is right now.


Comment 7 Lennart Poettering 2008-06-13 19:19:12 UTC
(In reply to comment #6)
> > Yes, they are plugins. Moving plugins to other directores is a pain to do in a
> > thread-safe manner with the mess that is ltdl, because you need to patch the
> > search path.
> 
> Sounds like another good reason not to use ltdl, if you ask me. 

This code is supposed to be portable. To have plugins working in a portable way
is either using ltdl or glib. To appease the KDE people and ease porters to
windows I kept glib out. So I am stuck with stupid ltdl, and I am not going to
maintain my own replacement for ltdl. 

> > I'd prefer leaving it the way it is for now. We can move that
> > around whenever we want in the future. 
> 
> I'd be more inclined to agree if this was legacy code. But this is brandnew. If
> we don't get it right now, it'll likely broken forever. Don't you think ?

Hmm, you might be right on that one. I will try to fix this. It's going to be
messy though. I am not really how the solution for this I use in PulseAudio
works. But in PA the races don't really matter since it is not a library, and I
have perfect control from where i call the ltdl functions. But yepp. I'll fix
it. Stay tuned. 


Comment 8 Lennart Poettering 2008-06-13 23:26:44 UTC
Ok, all fixed now. Spec file at same place, srpm is new since i released a new
upstream version 0.3 for this:

http://0pointer.de/public/libcanberra-0.3-1.fc10.src.rpm

Comment 9 Matthias Clasen 2008-06-14 01:42:42 UTC
Almost perfect now. Just add

%dir %{_libdir}/libcanberra

and then you are good to go.

Comment 10 Lennart Poettering 2008-06-14 16:48:34 UTC
srpm and spec file updated. Everything good to go?

Comment 11 Matthias Clasen 2008-06-14 17:30:35 UTC
Yes, approved 

Comment 12 Lennart Poettering 2008-06-15 01:59:44 UTC
New Package CVS Request
=======================
Package Name: libcanberra
Short Description: Portable Sound Event Library
Owners: lennart
Branches: devel
InitialCC: 
Cvsextras Commits: yes


Comment 13 Kevin Fenzi 2008-06-16 16:09:04 UTC
cvs done.

Comment 14 Lennart Poettering 2008-06-16 18:06:22 UTC
Imported to CVS and built.


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