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

Summary: Review Request: libcanberra - Portable Sound Event Library
Product: [Fedora] Fedora Reporter: Lennart Poettering <lpoetter>
Component: Package ReviewAssignee: Matthias Clasen <mclasen>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting
Target Milestone: ---Flags: mclasen: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-06-16 18:06:22 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

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.