Bug 195221 - Review Request: pulseaudio: Improved Linux sound server
Review Request: pulseaudio: Improved Linux sound server
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 195222 195223 198088 198089
  Show dependency treegraph
 
Reported: 2006-06-14 11:21 EDT by Pierre Ossman
Modified: 2008-06-15 11:40 EDT (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-29 16:07:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to correct pkgconfig files for libdir on x86_64 (1.92 KB, patch)
2006-07-21 12:44 EDT, Toshio Kuratomi
no flags Details | Diff
patch spec to remove static libs and fix rpath breakage (2.16 KB, patch)
2006-07-21 19:44 EDT, Toshio Kuratomi
no flags Details | Diff
Patch to spec to build separate jack package (1.76 KB, patch)
2006-08-23 05:37 EDT, David Fraser
no flags Details | Diff

  None (edit)
Description Pierre Ossman 2006-06-14 11:21:13 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/polypaudio/polypaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/polypaudio/polypaudio-0.9.1-1.src.rpm
Description:
Polypaudio is a sound server for Linux and other Unix like operating 
systems. It is intended to be an improved drop-in replacement for the 
Enlightened Sound Daemon (ESOUND).
Comment 1 Pierre Ossman 2006-06-14 11:24:46 EDT
See also bug 174866.
Comment 2 Eric Moret 2006-06-30 14:16:21 EDT
Could you please add support for zeroconf? Here is what I get when building
polypaudio on a system with avahi-compat-howl-devel and
avahi-compat-libdns_sd-devel installed.

[...]
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Checking for unpackaged file(s): /usr/lib/rpm/check-files
/var/tmp/polypaudio-0.9.1-1-root-emoret
error: Installed (but unpackaged) file(s) found:
   /usr/bin/pabrowse
   /usr/lib/libpolyp-browse.a
   /usr/lib/libpolyp-browse.so
   /usr/lib/libpolyp-browse.so.0
   /usr/lib/libpolyp-browse.so.0.0.0
   /usr/lib/polypaudio-0.9/modules/libhowl-wrap.so
   /usr/lib/polypaudio-0.9/modules/module-zeroconf-publish.so


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/bin/pabrowse
   /usr/lib/libpolyp-browse.a
   /usr/lib/libpolyp-browse.so
   /usr/lib/libpolyp-browse.so.0
   /usr/lib/libpolyp-browse.so.0.0.0
   /usr/lib/polypaudio-0.9/modules/libhowl-wrap.so
   /usr/lib/polypaudio-0.9/modules/module-zeroconf-publish.so
Comment 3 Pierre Ossman 2006-06-30 14:50:34 EDT
Ah, sorry. Must have missed that. I'm waiting for the first PulseAudio release
so that I can do all the necessary name changes. I'll fix this at the same time.
Comment 4 Pierre Ossman 2006-07-09 06:45:09 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.2-1.src.rpm

Updated to the new name and added the zeroconf parts.
Comment 5 Jason Tibbitts 2006-07-09 13:43:01 EDT
Before I start reviewing this, I checked and I don't see anything resembling
"drzeus" or your name in the cvsextras group.  If you haven't been sponsored,
your review tickets need to block FE-NEEDSPONSOR.
Comment 6 Pierre Ossman 2006-07-09 14:03:09 EDT
I haven't. So I'll add the dependency.
Comment 7 Jason Tibbitts 2006-07-09 17:25:54 EDT
OK, I'll take a look over the various packages; there should be enough here to
show me what I need to know but if you've done some review work them please
include the links so that I can take a look.

Check http://fedoraproject.org/wiki/Extras/HowToGetSponsored for more
information on the sponsorship process.
Comment 8 Pierre Ossman 2006-07-10 01:52:58 EDT
I'm afraid I haven't done any reviews as of yet as I hadn't gotten that deeply
involved in the Extras project until now. I just figured that I, as one of the
main developers of PulseAudio, could give these packages the attention they need. :)
Comment 9 Eric Moret 2006-07-10 11:10:36 EDT
Building on Rawhide works fine but does not work on FC5 with the following 
error:

[emoret@fc5 pkgconfig]$ sudo ldconfig
[...]
gcc -shared  .libs/libpulse_browse_la-browser.o  -Wl,--rpath -
Wl,/home/emoret/rpmbuild/BUILD/pulseaudio-0.9.2/src/.libs ./.libs/libpulse.so -
lhowl -lcap -ldl -lm  -pthread -m32 -march=i386 -mtune=generic -Wl,-soname -
Wl,libpulse-browse.so.0 -o .libs/libpulse-browse.so.0.0.0
/usr/bin/ld: cannot find -lhowl
collect2: ld returned 1 exit status
make[3]: *** [libpulse-browse.la] Error 1
make[3]: Leaving directory `/home/emoret/rpmbuild/BUILD/pulseaudio-0.9.2/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/emoret/rpmbuild/BUILD/pulseaudio-0.9.2/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/emoret/rpmbuild/BUILD/pulseaudio-0.9.2'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.9607 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.9607 (%build)
[emoret@fc5 SPECS]$ locate avahi-compat-howl.pc
/usr/lib/pkgconfig/avahi-compat-howl.pc
[emoret@fc5 SPECS]$ cd /usr/lib/pkgconfig/
[emoret@fc5 pkgconfig]$ ls -la howl.pc
lrwxrwxrwx 1 root root 20 Jul 10 00:15 howl.pc -> avahi-compat-howl.pc
[emoret@fc5 pkgconfig]$ locate libhowl
/usr/lib/libhowl.so
[emoret@fc5 pkgconfig]$
Comment 10 Eric Moret 2006-07-10 11:13:50 EDT
Also I have noticed that pabrowse is part of module-zeroconf while pax11publish 
is in utils even if it depends on module-x11. Don't you think it might make 
more sense to either include pabrowse in utils or move pax11publish in module-
x11 for sake of consistency?
Comment 11 Pierre Ossman 2006-07-10 16:35:29 EDT
pabrowse is packaged with lib-zeroconf (client side), not module-zeroconf
(server side). There is no client side package for X11 integration (it's in the
client lib unconditionally) so it made no sense to put pax11publish in its own
package.

The broken build is an error in the avahi package which has forgotten to add a
dependency from the devel package to the "main" one. Filed as bug 198282.
Comment 12 Eric Moret 2006-07-11 03:24:27 EDT
Actually you must be right regarding pabrowse :) Something else I noticed on 
rawhide, I don't get the nice icons I can see on the screenshot for the GUI 
interfaces in the default gnome fedora theme. IE, I see a red cross in the 
trayicon when launching padevchooser instead of the speaker from the main 
site's screenshot. Likewise no icon in Applications ! Sound & Video Menu for 
Pulseaudio Device Chooser. I must be missing something but what?
Comment 13 Pierre Ossman 2006-07-11 11:25:46 EDT
PulseAudio follows the icon naming spec and GNOME isn't fully compliant yet. So
in good time my friend :)

(I have no insight into Bluecurve with regard to the naming spec)
Comment 14 Rahul Sundaram 2006-07-17 10:14:03 EDT
There is a plan to introduce a new set of icons which follow the freedesktop
icon spec in FC6. Details at
http://fedoraproject.org/wiki/Artwork/BluecurveAndBeyond
Comment 15 David Nielsen 2006-07-18 20:08:29 EDT
When compiling the src.rpm on current Development I get the following error:

gcc -shared  .libs/libpulsecore_la-channelmap.o .libs/libpulsecore_la-error.o
.libs/libpulsecore_la-mainloop.o .libs/libpulsecore_la-mainloop-api.o
.libs/libpulsecore_la-mainloop-signal.o .libs/libpulsecore_la-sample.o
.libs/libpulsecore_la-timeval.o .libs/libpulsecore_la-utf8.o
.libs/libpulsecore_la-util.o .libs/libpulsecore_la-volume.o
.libs/libpulsecore_la-xmalloc.o .libs/libpulsecore_la-autoload.o
.libs/libpulsecore_la-cli-command.o .libs/libpulsecore_la-cli-text.o
.libs/libpulsecore_la-client.o .libs/libpulsecore_la-conf-parser.o
.libs/libpulsecore_la-core.o .libs/libpulsecore_la-core-scache.o
.libs/libpulsecore_la-core-subscribe.o .libs/libpulsecore_la-core-util.o
.libs/libpulsecore_la-dynarray.o .libs/libpulsecore_la-g711.o
.libs/libpulsecore_la-hashmap.o .libs/libpulsecore_la-idxset.o
.libs/libpulsecore_la-log.o .libs/libpulsecore_la-mcalign.o
.libs/libpulsecore_la-memblock.o .libs/libpulsecore_la-memblockq.o
.libs/libpulsecore_la-memchunk.o .libs/libpulsecore_la-modargs.o
.libs/libpulsecore_la-modinfo.o .libs/libpulsecore_la-module.o
.libs/libpulsecore_la-namereg.o .libs/libpulsecore_la-pid.o
.libs/libpulsecore_la-pipe.o .libs/libpulsecore_la-play-memchunk.o
.libs/libpulsecore_la-poll.o .libs/libpulsecore_la-props.o
.libs/libpulsecore_la-queue.o .libs/libpulsecore_la-random.o
.libs/libpulsecore_la-resampler.o .libs/libpulsecore_la-sample-util.o
.libs/libpulsecore_la-sconv.o .libs/libpulsecore_la-sconv-s16be.o
.libs/libpulsecore_la-sconv-s16le.o .libs/libpulsecore_la-sink
.o .libs/libpulsecore_la-sink-input.o .libs/libpulsecore_la-sioman.o
.libs/libpulsecore_la-sound-file.o .libs/libpulsecore_la-sound-file-stream.o
.libs/libpulsecore_la-source.o .libs/libpulsecore_la-source-output.o
.libs/libpulsecore_la-strbuf.o .libs/libpulsecore_la-tokenizer.o
.libs/libpulsecore_la-core-error.o  /usr/lib/libltdl.so -lsamplerate -lsndfile
-loil-0.3 -lcap -ldl -lm  -pthread -Wl,-soname -Wl,libpulsecore.so.0 -o
.libs/libpulsecore.so.0.0.1
/usr/lib/libltdl.so: could not read symbols: File in wrong format
collect2: ld returned 1 exit status
make[3]: *** [libpulsecore.la] Error 1
make[3]: Leaving directory `/usr/src/redhat/BUILD/pulseaudio-0.9.2/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/usr/src/redhat/BUILD/pulseaudio-0.9.2/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/usr/src/redhat/BUILD/pulseaudio-0.9.2'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.26234 (%build)
Comment 16 Jason Tibbitts 2006-07-20 20:12:44 EDT
I had no problems building in rawhide, but rpmlint is very unhappy:

Tons of rpath warnings:
  E: pulseaudio binary-or-shlib-defines-rpath  
/usr/lib64/pulse-0.9/modules/libprotocol-esound.so
['/usr/lib64/pulse-0.9/modules/', '/usr/lib64']
and 69 others.  The usual libtool hack doesn't work.

This looks like a script but isn't executable:
  E: pulseaudio non-executable-script /etc/pulse/default.pa 0644

Some setuid bits:
  E: pulseaudio setuid-binary /usr/bin/pulseaudio root 04755
  E: pulseaudio non-standard-executable-perm /usr/bin/pulseaudio 04755
which will need to be discussed to make sure we're not getting into any security
issues.

Some no-documentation warnings:
  W: pulseaudio-devel no-documentation
  W: pulseaudio-lib-glib no-documentation
  W: pulseaudio-lib-zeroconf no-documentation
  W: pulseaudio-module-alsa no-documentation
  W: pulseaudio-module-x11 no-documentation
  W: pulseaudio-module-zeroconf no-documentation
  W: pulseaudio-utils no-documentation
which are probably OK but I'll have to check.

This is problematic:
  E: pulseaudio-utils invalid-soname /usr/lib64/libpulsedsp.so libpulsedsp.so
but unfortunately I'm not entirely sure how to fix it.
Comment 17 Pierre Ossman 2006-07-21 07:01:01 EDT
(In reply to comment #16)
> I had no problems building in rawhide, but rpmlint is very unhappy:
> 
> Tons of rpath warnings:
>   E: pulseaudio binary-or-shlib-defines-rpath  
> /usr/lib64/pulse-0.9/modules/libprotocol-esound.so
> ['/usr/lib64/pulse-0.9/modules/', '/usr/lib64']
> and 69 others.  The usual libtool hack doesn't work.

This is a libtool/gcc misfeature. I talked to Ralf Wildenhues about it, but he
didn't have any way of solving it (short of bastardising libtool). It's just the
way things are on multi-arch for now.

> 
> This looks like a script but isn't executable:
>   E: pulseaudio non-executable-script /etc/pulse/default.pa 0644
> 

The hash bang is mostly there to show that it can be used that way. As it is
located in /etc, I think we should keep the execute bit off.

> Some setuid bits:
>   E: pulseaudio setuid-binary /usr/bin/pulseaudio root 04755
>   E: pulseaudio non-standard-executable-perm /usr/bin/pulseaudio 04755
> which will need to be discussed to make sure we're not getting into any security
> issues.

Sure. Pulse only uses its root privileges to change to realtime scheduling, then
drops them. Access control is based on who is in the 'realtime' group.

> 
> Some no-documentation warnings:
>   W: pulseaudio-devel no-documentation
>   W: pulseaudio-lib-glib no-documentation
>   W: pulseaudio-lib-zeroconf no-documentation
>   W: pulseaudio-module-alsa no-documentation
>   W: pulseaudio-module-x11 no-documentation
>   W: pulseaudio-module-zeroconf no-documentation
>   W: pulseaudio-utils no-documentation
> which are probably OK but I'll have to check.
> 

The docs are in the other packages.

> This is problematic:
>   E: pulseaudio-utils invalid-soname /usr/lib64/libpulsedsp.so libpulsedsp.so
> but unfortunately I'm not entirely sure how to fix it.

It's not a "real" lib, so it has no use for versioning. It's the active part of
an LD_PRELOAD hack to provide OSS emulation. As such, the error is incorrect and
the lib is just fine.
Comment 18 Toshio Kuratomi 2006-07-21 12:43:50 EDT
License should be both GPL and LGPL. (See the LICENSE file for details)

pkgconfig files won't do the right thing on x86_64.  Will attach a patch for that.
Comment 19 Toshio Kuratomi 2006-07-21 12:44:58 EDT
Created attachment 132822 [details]
Patch to correct pkgconfig files for libdir on x86_64
Comment 20 Toshio Kuratomi 2006-07-21 16:44:55 EDT
Is there a reason to ship static libraries with this package?
Comment 21 Pierre Ossman 2006-07-21 17:05:30 EDT
(In reply to comment #20)
> Is there a reason to ship static libraries with this package?

Other than completeness, no.
Comment 22 Pierre Ossman 2006-07-21 17:43:29 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.3-1.src.rpm

Updated with a new version. Also fixed the license and pc issue pointed out.
(pc fix also committed upstream)
Comment 23 Toshio Kuratomi 2006-07-21 19:37:21 EDT
* Should get rid of the static libraries as well.

Comment #17:
>>   E: pulseaudio-utils invalid-soname /usr/lib64/libpulsedsp.so libpulsedsp.so
>> but unfortunately I'm not entirely sure how to fix it.
>
> It's not a "real" lib, so it has no use for versioning. It's the active part of
> an LD_PRELOAD hack to provide OSS emulation. As such, the error is incorrect 
> and the lib is just fine.

libesddsp and libartsdsp are versioned::
  $ objdump -p /usr/lib64/libartsdsp.so /usr/lib64/libesddsp.so.0|grep SONAME
    SONAME      libartsdsp.so.0
    SONAME      libesddsp.so.0

I don't know how these libraries do their tricks, but is there any improbable
case when you might want to have it versioned?  The kernel upgrades what it
expects to receive on /dev/dsp and you want to provide both the older interface
and newer interface to programs?
Comment 24 Toshio Kuratomi 2006-07-21 19:44:26 EDT
Created attachment 132852 [details]
patch spec to remove static libs and fix rpath breakage

Here's a spec patch for two of the issues.  It removes static libraries and
fixes rpath.  Note that some of the files in the modules directory,
%{_libdir}/pulse-0.9/modules, need to have an rpath to that directory because
they have dependencies on other shared objects in there.  Before, they had a
second rpath set on /usr/lib64 which was wrong.  Also, some of the modules do
not have dependencies on anything within that directory.  Therefore, they
should have no rpath set.  This patch should make all of that happy (and
rpmlint should no longer complain about rpaths.)
Comment 25 Pierre Ossman 2006-07-21 19:47:35 EDT
Why kill of the static libs? Although dynamic linking is preferred, I see no
point in preventing those that want to do a static link.

The versioning in lib*dsp is completely useless and can only be motivated by
defeating warnings like those in rpmlint. The .so shouldn't be seen as a lib,
more like a program. The reason it is a lib is because that's the way LD_PRELOAD
works, not because you want the properties a shared library gives you.
Comment 26 Anthony Green 2006-07-21 20:08:46 EDT
(In reply to comment #25)
> Why kill of the static libs? Although dynamic linking is preferred, I see no
> point in preventing those that want to do a static link.

Policy....
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

I don't know the full reasoning behind the policy (it would be nice if the
guidelines came with a raionale document), but the obvious reason is easy enough
to guess.
Comment 27 Toshio Kuratomi 2006-07-21 20:17:51 EDT
Static libs are security hole.  If I link my program against the static versions
of a library and a vulnerability is later fixed in the library I will still be
carrying around vulnerable code until I recompile against the newer version. 
With dynamic libs, only the library packager needs to be on the ball about
finding security holes and making updates.  The consumers of the library get the
hole closed after they update the library.

Versioning: Bear with me.  I'm trying to imagine if there's any reason that
versioning could be useful so we know we're not introducing broken behaviour. 
Since arts and esd both do it they either have thought of some corner case where
it's useful or they're both broken.  The latter is very likely (they're broken
in many other ways) but we want to make sure we actually are smarter than our
predecessors rather than falling into a problem that they avoided.
Comment 28 David Nielsen 2006-07-21 20:19:28 EDT
(In reply to comment #24)
> Created an attachment (id=132852) [edit]
> patch spec to remove static libs and fix rpath breakage
> 
> Here's a spec patch for two of the issues.  It removes static libraries and
> fixes rpath.  Note that some of the files in the modules directory,
> %{_libdir}/pulse-0.9/modules, need to have an rpath to that directory because
> they have dependencies on other shared objects in there.  Before, they had a
> second rpath set on /usr/lib64 which was wrong.  Also, some of the modules do
> not have dependencies on anything within that directory.  Therefore, they
> should have no rpath set.  This patch should make all of that happy (and
> rpmlint should no longer complain about rpaths.)

The patch succesfully fixes the compile issue I pointed out earlier, thank you
very much.

Any hope of getting gst-pulse packaged as well so pulseaudio will be more useful
on a default FC desktop?
(http://0pointer.de/lennart/projects/gst-pulse/)
Comment 29 Pierre Ossman 2006-07-23 16:42:34 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.3-2.src.rpm

Updated with Toshio's patch.

About the versioning... The reason we have versions on libs is so that:

 a) We can have multiple versions of a lib on the system.
 b) If we break ABI, applications depending on this lib won't be able to load it
as it will have a different ver.

Now apply this to libpulsedsp.so and you'll see that neither matters. The
primary reason being that the only "user" of the lib is the padsp script that
needs to know the name when setting up the LD_PRELOAD variable. However, as the
two are part of one package, there is no need for version handling. Adding it
would be the equivalent of versioning glade files for every GTK application.

If a valid case for versions presents itself, then I'll gladly support adding
it. But if it is only to shut up an incorrect warning, then things should stay
the way they are upstream.
Comment 30 Toshio Kuratomi 2006-07-24 14:17:44 EDT
It could be that neither matters but the reasoning is flawed.  In LTSP thin
client installations (where you need to have a networked audio server in order
to get sound) it is common to set up the LD_PRELOAD variable to load the dsp
redirecting library system wide.  So the padsp script is never used in this case.

I think the real question is whether the calls that libpulsedsp is overriding
(AFAICT: _ioctl, _close, _open, _fopen, _open64, _fopen64, _fclose, _access )
will ever change their ABI.  And if they do, will we need to provide for
concurrent old and new versions or will the change be such that everything will
have to upgrade with no backwards compat-possible.

I don't usually work at this level, so feel free to correct any misconceptions I
might have as to how libpulsedsp works and to tell me what will happen in the
case above.  
Comment 31 Anthony Green 2006-07-24 14:48:49 EDT
(In reply to comment #30)
> It could be that neither matters but the reasoning is flawed.  In LTSP thin
> client installations (where you need to have a networked audio server in order
> to get sound) it is common to set up the LD_PRELOAD variable to load the dsp
> redirecting library system wide.  So the padsp script is never used in this case.

Doesn't this result in disable prelinking system wide?

> I think the real question is whether the calls that libpulsedsp is overriding
> (AFAICT: _ioctl, _close, _open, _fopen, _open64, _fopen64, _fclose, _access )
> will ever change their ABI.  

Yes, symbol versioning is something to worry about. 

LD_PRELOAD is a neat trick but I've learned the hard way that it's basically a
bad idea for anything other than debugging.  I'm not all that familiar with
pulseaudio yet, but isn't there some other way to get similar functionality?
Comment 32 Toshio Kuratomi 2006-07-24 15:16:09 EDT
(In reply to comment #31)
> (In reply to comment #30)
> > In LTSP thin client installations (where you need to have a networked audio
> > server in order to get sound) it is common to set up the LD_PRELOAD variable 
> > to load the dsp redirecting library system wide.
> 
> Doesn't this result in disable prelinking system wide?
> 
I've never thought of that but it could.  I'll mention this on the k12ltsp list
and see what response comes back.

> LD_PRELOAD is a neat trick but I've learned the hard way that it's basically a
> bad idea for anything other than debugging.  I'm not all that familiar with
> pulseaudio yet, but isn't there some other way to get similar functionality?
> 
Monty mentioned on IRC that he's working on fusd (Device files from userspace.)
 He thinks that will allow pulseaudio to create the device files rather than
having to load a library that overrides calls to /dev/dsp with calls to
pulseaudio instead.  Without that I think all prior art is to use LD_PRELOAD in
this manner.  (Although thin client is the only place I'm aware of them doing it
system-wide; most places use the {esd,arts,pa}dsp wrapper to target specific
applications.)
Comment 33 Pierre Ossman 2006-07-24 21:46:29 EDT
(In reply to comment #30)
> It could be that neither matters but the reasoning is flawed.

The reasoning is sound, you just expect the versioning to do more magic than it
does.

> 
> I think the real question is whether the calls that libpulsedsp is overriding
> (AFAICT: _ioctl, _close, _open, _fopen, _open64, _fopen64, _fclose, _access )
> will ever change their ABI.  And if they do, will we need to provide for
> concurrent old and new versions or will the change be such that everything will
> have to upgrade with no backwards compat-possible.

Versioning does not work on this level. If we assume that _open changed it's
ABI, then libc.so.6 would become libc.so.7. Still, this would mean nothing to
libpulsedsp.so as the only one who knows about it is padsp (or some other script
setting the LD_PRELOAD variable). If we have a mix of applications needing
libc.so.6 and libc.so.7 then versioning libpulse.so wouldn't solve our problem.
We, as users, would need to provide info on which ABI is in place. There
wouldn't be any need for two libpulse.so though, as it could be designed to
handle both ABI:s, provided it gets information about which it should use.
Comment 34 Toshio Kuratomi 2006-07-25 14:10:48 EDT
Thanks for explaining.  If I understand correctly, if an ABI change occurs,
pulseaudio will enhance the pulsedsp library to receive either the old or new
ABI, convert the call properly based on some external information (config file,
ENVVAR, etc), and then send it on to the daemon.  If that's the pulseaudio
team's plan for dealing with ABI changes, then I can't think of any other
instances versioning would come in handy at the moment.
Comment 35 Callum Lerwick 2006-08-05 23:17:48 EDT
Given that libpulsedsp.so is a preload hack library, I don't think it should be
in %{_libdir}, and thus in the default linker path. It belongs in a subdir.
Comment 36 Pierre Ossman 2006-08-06 06:58:51 EDT
That would be nice. Unfortunately it isn't possible as we want to support
multi-arch, which requires us to put it in lib[64] and let ld.so find it.
Comment 37 Rex Dieter 2006-08-17 15:22:09 EDT
I can try reviewing this...

Pierre, do you have any notions/intentions to maintain more than just this in
Fedora Extras?

I ask mainly because as your first reviewer, I'll need to also sponsor you.
Comment 38 Rex Dieter 2006-08-17 15:25:43 EDT
Duh, just checked all(most?) other bugs depending on this one, and they're
mostly yours too.  Nevermind the silly question.  
Comment 39 Rex Dieter 2006-08-17 16:10:42 EDT
MUSTFIX: lib-devel contains pkgconfig file(s), so it ought to:
Requires: pkgconfig

Can you explain/justify the existence of *both* a pulseaudio-devel and a
pulseaudio-libs-devel pkg?  

I'm also inclined to say that there's needless complexity splitting out separate
packages for module-alsa, lib-glib2, lib-zeroconf subpkgs.  These are pretty
low-level, core libraries that will (should?!) be present on any
audio-capable/desktop-config machine.
Comment 40 Pierre Ossman 2006-08-18 02:12:48 EDT
(In reply to comment #39)
> MUSTFIX: lib-devel contains pkgconfig file(s), so it ought to:
> Requires: pkgconfig

Hmm... is this to get the correct directory structure? Because a owner of .pc
files has no need for the pkgconfig command (the users of the .pc files are the
ones that need the command).

> 
> Can you explain/justify the existence of *both* a pulseaudio-devel and a
> pulseaudio-libs-devel pkg?  
> 

Of course. :)

pulseaudio-devel is the headers and libraries for building modules for the
server. pulseaudio-libs-devel is the headers and libraries for building
PulseAudio clients.

> I'm also inclined to say that there's needless complexity splitting out separate
> packages for module-alsa, lib-glib2, lib-zeroconf subpkgs.  These are pretty
> low-level, core libraries that will (should?!) be present on any
> audio-capable/desktop-config machine.

I couldn't find any reasonable list of libraries that are "always present", so I
went with the approach of least dependencies.

Note though that a machine running pulseaudio server/clients isn't necesariliy a
desktop machine. You could have a machine hooked up to output sound on your
living room stereo, or you could have a dedicated mp3 player (HTPC perhaps) that
you want to stream data somewhere.
Comment 41 Paul Howarth 2006-08-18 03:48:02 EDT
(In reply to comment #40)
> (In reply to comment #39)
> > MUSTFIX: lib-devel contains pkgconfig file(s), so it ought to:
> > Requires: pkgconfig
> 
> Hmm... is this to get the correct directory structure? Because a owner of .pc
> files has no need for the pkgconfig command (the users of the .pc files are the
> ones that need the command).

A .pc file is useless without pkgconfig, hence packages providing .pc files
should require pkgconfig. And even if you don't buy that argument, the directory
that the .pc file is installed into is owned by the pkgconfig package, which is
another reason why it must be required.

Moreover, libpulse-mainloop-glib.pc in the pulseaudio-lib-devel package contains:

Requires: libpulse glib-2.0

glib-2.0.pc is provided by glib2-devel so pulseaudio-lib-devel should have a
dependency on glib2-devel.
Comment 42 Rex Dieter 2006-08-18 10:53:49 EDT
Thanks for Paul's helpful analysis, (in addition to MUSTFIX item from comment 
#39):

2. MUSTFIX, pulseaudio-lib-devel needs:
Requires: glib2-devel

Since you're upstream dev, it may be worth perusing pulseaudio's headers to 
see if dependancies on other pkgs exist as well (I'm betting not, but you 
never know).
Comment 43 Pierre Ossman 2006-08-20 06:45:00 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.4-2.src.rpm

Update to 0.9.4 and the dependency fix included. I haven't touched the number of
subpackages as I want some more comments on the current state. These are the
current split out packages:

pulseaudio-lib-glib2:
Client side dependency on GLib 2.0.

pulseaudio-lib-zeroconf:
Client side dependency on Avahi.

pulseaudio-module-alsa:
Server side dependency on ALSA libs

pulseaudio-module-lirc:
Server side dependency on LIRC libs.

pulseaudio-module-x11:
Server side dependency on X11, SM and ICE libs.

pulseaudio-module-zeroconf:
Server side dependency on Avahi.


Personally, I don't care for packages that pull in everything and the kitchen
sink because of some optional components, hence all these sub-packages. I am
open to input if the concensus is that some/all of these are excessive.
Comment 44 Rex Dieter 2006-08-21 08:06:00 EDT
0. Comment in specfile:
# configure --disable-static had no effect; delete manually.
FYI, this is most likely due to your using LIBTOOL=/usr/bin/libtool
No suggestion here, just an FYI. (:

1.  SHOULD/COULD:  You could use the %{fedora} macro (defined in Fedora's
buildsystem), to conditionalize this bit:
# FC5
BuildRequires:	libXt-devel, xorg-x11-proto-devel
# FC4 or earlier
# BuildRequires:	xorg-x11-devel

into something like:
%if "%{?fedora}" > "4"
BuildRequires:	libXt-devel, xorg-x11-proto-devel
%else
BuildRequires:	xorg-x11-devel
%endif

But I'll leave the choice of doing this up to you (you're the one that'll have
to maintain it afterall).


2.  Regarding split-out server/client libs/modules.  Will pulseaudio apps link
with these (and automatically include them as dependancies)?  
If yes and/or dependancies are handled automatically for end-users, end of
problem.  
If no, how will users' get these extra dependancies installed on their machines
(other than doing so manually)?


Address this last 2 issues, and I'll APPROVE this (and sponsor you).
Comment 45 David Fraser 2006-08-21 14:09:12 EDT
URL in comment #43 should be
http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.4-1.src.rpm (-1 not -2)
Comment 46 Pierre Ossman 2006-08-22 16:14:20 EDT
0. It was Toshio that did those additions. The Red Hat libtool was added to
avoid getting a RPATH on x86_64.

1. I'm blisfully unaware to the the available macros. I'll get that into the
next revision, thanks. :)

2. Yes and no, we'll have a look, package by package:

pulseaudio-lib-glib2:
This is for GLib 2 applications that want to easily integrate with the
PulseAudio client lib. As such, they'll have a dependency on the .so file in
this package and everything will be peachy.

pulseaudio-lib-zeroconf:
This is similar as it provides a shared object for automatically finding
PulseAudio servers and connecting to them. It does, however, also contain a
command line tool that might be of interest to the users. But as apt and yum can
fetch based on knowing that tools name (pabrowse), users should be able to pull
it in without much trouble.

pulseaudio-module-alsa:
Failure to have this package will result in error messages about failing to load
modules, provided their configuration states that the ALSA modules should be loaded.
As ALSA is the standard API, I suppose we could remove this subpackage.

pulseaudio-module-lirc:
Same result here. This module provides nothing as far as clients are concerned
(it allows you to directly control the server via IR), so there will be an
obvious error for those trying to use the function without having the package.

pulseaudio-module-x11:
This is more subtle from a user's point of view. This module provides some extra
authentication features (allows you to set the security cookie as a X11
property). I am a strong proponent of keeping this subpackage though as X11 can
be a big dependency.

pulseaudio-module-zeroconf:
This is a server-side feature and will therefore result in complaints about
missing modules when trying to use it without the package.


So I can accept removing the ALSA subpackage, but the others should stay.
Comment 47 Rex Dieter 2006-08-22 16:20:34 EDT
Fair reasoning.  So, the conclusion is to (only) drop the module-alsa subpkg 
(to be subsumed into -lib?), and I think this should be good to go.
Comment 48 Pierre Ossman 2006-08-22 16:49:45 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.4-2.src.rpm

Remove the ALSA subpackage.

The %{?fedora} macro isn't defined in my rpm though, so I couldn't include that
part. Is it perhaps only present on the build system?
Comment 49 David Fraser 2006-08-23 04:53:41 EDT
Hmmm ... the URLs in comment #48 still don't work for me, pulseaudio.spec seems
to be the version from 2006-08-20 and pulseaudio-0.9.4-2.src.rpm doesn't exist
Curiously http://homes.drzeus.cx/~drzeus/public.html seems to contain the latest
pulseaudio.spec

Am I the only one having these problems?
Comment 50 Pierre Ossman 2006-08-23 05:02:32 EDT
I guess it was a bit late when I uploaded those. They should work now. :)
Comment 51 David Fraser 2006-08-23 05:37:16 EDT
Created attachment 134695 [details]
Patch to spec to build separate jack package

When I build pulseaudio, because of having jack-audio-connection-kit installed,
the build complains about unpackaged modules:
/usr/lib/pulse-0.9/modules/module-jack-sink.so
/usr/lib/pulse-0.9/modules/module-jack-source.so

Attached a patch to include these in a separate pulseaudio-module-jack package,
if an optional --with jack build switch is used.

I'm not sure if this is the best way to do it or if there is a way to detect an
optional dependency and use it automatically. If you have jack-audio-connection
kit installed and build without "--with jack" the build will fail, so I'm sure
there's a better way - any suggestions?
Comment 52 Pierre Ossman 2006-08-23 07:18:18 EDT
Spec URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio.spec
SRPM URL: http://homes.drzeus.cx/~drzeus/pulseaudio/pulseaudio-0.9.4-3.src.rpm

JACK should always be built, so this version has more or less your patch (just
no if:s).
Comment 53 Rex Dieter 2006-08-23 11:10:12 EDT
Re: comment #48:
> The %{?fedora} macro isn't defined in my rpm though, so I couldn't 
> include that part. Is it perhaps only present on the build system?

Yes, the buildsystem defines it, as well as a few others, like %{dist}.  You 
can similate that via:
rpmbuild --define "fedora 5" -bb pulseaudio.spec

That said, package looks good, APPROVED.  Now I have to figure out this 
sponsorship thing. (:
Comment 54 Pierre Ossman 2006-08-25 09:44:19 EDT
I'm working on getting an account up, but the CLA system doesn't seem to like
me. I've sent a mail to the suggested email address for help, but so far no reply.

I'll keep you posted as things progress...
Comment 55 Pierre Ossman 2006-08-28 02:51:50 EDT
Ok, the CLA is now accepted. I've added myself to the cvsextras group, so feel
free to approve that. :)
Comment 56 Rex Dieter 2006-08-28 07:42:17 EDT
OK, you are now sponsored.  Import pulseaudio into cvs, and mark this as
Resolved->NextRelease when done.
Comment 57 Warren Togami 2006-08-28 11:04:32 EDT
Note that we plan on replacing esound in FC7 with pulseaudio.  pulseaudio is
supposed to be some kind of drop-in replacement to esound.  So for < FC7 we need
pulseaudio to co-exist without conflicting or removing esound, but FC7 we need
to design a smooth upgrade path for both the sound server and packages that
currently depend on it by name.
Comment 58 Pierre Ossman 2006-08-28 12:44:47 EDT
So it will be moved into core then?
Comment 59 Rex Dieter 2006-08-28 12:47:02 EDT
Per Warren's comment, I'd venture -> Core sometime after FC-6 release and 
before FC-7. (:
Comment 60 Warren Togami 2006-08-28 14:04:09 EDT
Does this current version co-exist with esound without causing conflicts?
Comment 61 Pierre Ossman 2006-08-28 14:42:23 EDT
No direct conflicts no. Only one can have the local socket though of course.

There aren't really any ways pulse can conflict with esound, apart from
replacing "esd" with the esdcompat script.
Comment 62 Lennart Poettering 2007-07-30 13:36:20 EDT
Package Change Request
======================
Package Name: pulseaudio
Updated Fedora Owners: lpoetter@redhat.com,drzeus-bugzilla@drzeus.cx

I am upstream of this package and Pierre agreed to co-maintain this package with
me from now on.
Comment 63 Jason Tibbitts 2007-07-30 13:39:27 EDT
CVS done.
Comment 64 Lubomir Kundrak 2008-04-14 13:08:24 EDT
Maintainer is OK with the change as per previous conversation with him.

Package Change Request
======================
Package Name: pulseaudio
New Branches: EL-5
Owners for new branch: lkundrak
cvsextras commits for new branch: yes
Comment 65 Kevin Fenzi 2008-04-14 15:36:52 EDT
cvs done.

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