Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Request For Testing] pkgs: libpulseaudio -> libcardiacarrest; nixos: override when running PA daemon #36881

Closed
wants to merge 1 commit into from

Conversation

oxij
Copy link
Member

@oxij oxij commented Mar 12, 2018

What?

libcardiacarrest (https://github.com/oxij/libcardiacarrest) is a trivial implementation of libpulse* PulseAudio library API that unconditionally (but gracefully) fails to connect to the PulseAudio daemon and does nothing else. See https://github.com/oxij/libcardiacarrest/blob/master/README.org for more info.

With this we switch to linking against libcardiacarrest instead of the vanilla libpulseaudio by default. NixOS then overrides this with the vanilla libpulseaudio via LD_LIBRARY_PATH when user enables the daemon service.

Motivation
  • If you do run PulseAudio daemon you shouldn't notice anything (except, maybe, for one of the Cons below until things will get fixed there).

Pros:

Cons:

  • Since it's a different implementation, it makes some buggy software crash sometimes. Which can be inconvenient. But it will print helpful tracing messages when it does so. Also overriding this with the vanilla libpulseaudio on app-by-app basis for a temporary workaround is pretty trivial.
  • Apps than override LD_LIBRARY_PATH instead of appending into it will need fixing when running with PulseAudio daemon. Or we can do something like glibc: add NIX_LD_LIBRARY_PATH #31263 (but, hopefully, a bit better). Or you can just override the broken app with the vanilla libpulseaudio and forget about it.
Related things

This is a better version of what I wanted to have with #35374 which did the same with libpressureaudio. The problem with linking against libpressureaudio by default is that many apps just start using apulse emulation of PA API and completely ignore native ALSA. This hurts performance, ignores cool things you can do with your asound.conf, and puts pressure on apulse to emulate more of the PA API, which could eventually turn apulse into a monster libpulseaudio already is. libcardiacarrest is tiny (even by comparison to apulse, not just to libpulseaudio), all it does is it reports to the caller that PulseAudio daemon is unavailable, all the apps I tested then just switch to other output plugins like ALSA or JACK. This closes #35374.

What I did
  • I built a lot (~60% of nixpkgs and counting) of things with this and everything that built before seems to build now.
  • I'm running several machines with minimalistic DEs on top of this for a week and everything is perfect.
  • I'm running a machine with Xfce DE on top of this, and everything is perfect, but I used it maybe a couple of hours this week, so it may have issues I didn't encounter.
  • I tried running KDE with this, everything seems to be perfect, but I'm not a KDE user, so who knows.
  • I tried building Gnome3, but it failed even before applying this patch, so, meh.
What I want you to do
  • Running with the actual PulseAudio daemon. I have a report that it works with one of the previous versions of this branch, but I'm not running PulseAudio myself.
  • Building everything you use against this. Maybe something breaks. Can I have a hydra job for this?
  • Run everything you use under this. Maybe something crashes.
Merge plan

After somebody else can confirm that running with PulseAudio daemon works, I think, in principle, you can merge this immediately because I'm pretty sure it doesn't break anything sufficiently common and overriding with the vanilla libpulseaudio on app-by-app basis for uncommon things (while things get fixed, or just forever) is trivial.

You could keep it here for a week or two anyway so that people could test it, but the practice shows that people don't test Nixpkgs PRs until they get into master, so, inevitably, there are going to be surprises. I'd like this to get into master ASAP to be over the surprises ASAP.

Hydra job would be nice, though.

So. Study this branch. Study the source of libcardiacarrest. Confirm I'm not doing anything stupid or evil. Try this with PulseAudio daemon. Merge.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 12, 2018
@grahamc
Copy link
Member

grahamc commented Mar 12, 2018

This should probably go through an RFC first

… PA daemon

This satisfies both people that want to purge every single bit of PulseAudio
from their systems for ethical, religious, and/or security reasons and those who
want to run the daemon for convenience reasons.

This also significantly reduces closure size when not running PulseAudio daemon
since `libcardiacarrest` is tiny.

NOTE: With this in place I

- built everything I use, Xfce, KDE, most of GNOME3,
- tested everything I use, Xfce, KDE, official PulseAudio tools like
  pavucontrol, etc,
- verified that some other things I don't want to build (like Chromium) would
  work with this by looking at their sources,

and everything seems to work (i.e. gracefully fails to use PulseAudio).

However, I'm almost sure this will break things I didn't build and test.
PulseAudio API is huge and full of surprises, hence many application authors
fail use it correctly (and they are not alone, even the likes of Mozilla fail to
properly check for PA errors in Firefox).

If this commit breaks your thing, please, don't just revert this, instead
override `libpulseaudio` for your thing with `libpulseaudio-vanilla` and then
let me know about it so that I could fix an "ignores return values" bug in your
thing or add some missing pieces to `libcardiacarrest` to make your thing happy.
Thank you.
@oxij oxij force-pushed the pkgs/the-cardiacarrest branch from baa7667 to 7a29afd Compare March 12, 2018 21:00
@oxij oxij changed the title [Request For Testing] pkgs: libpulseaudio -> libcardiacarrest; nixos: override when running a daemon [Request For Testing] pkgs: libpulseaudio -> libcardiacarrest; nixos: override when running PA daemon Mar 12, 2018
@oxij
Copy link
Member Author

oxij commented Mar 12, 2018

Well, I can copy-paste the OP into an RFC, but what is the point? It is a running code you can test already. No work except testing this needs to be done, really. I did all of it already. There's no real downsides to merging this (see the OP). And there's also not much of any kind of "design" involved anywhere. All this does, really, is it just replaces the pa_context_connect call of libpulse with a stub that always returns errors and stubs out some other PA functions that are needed to actually get apps to making that call. The whole thing is trivial, it's just PA API is enormous and under-specified and many things fail to use it correctly. Hence it took me awhile to get to this state.

@grahamc
Copy link
Member

grahamc commented Mar 13, 2018

My chief complaint is this PR is it is setting a general policy and direction for NixOS, and causing us to take a particular stance. It is not the code at issue here -- it is the roadmap-setting-disguised-as-code.

@7c6f434c
Copy link
Member

@grahamc While I agree that your efforts to get processes working in Nix* are valuable and more succesful than can be believed, I am not sure we already have an RFC process that produces outcomes — any outcomes.

@abbradar
Copy link
Member

abbradar commented Mar 13, 2018

Several points (disclosure: I use PulseAudio and see merits in it for a desktop user but want to keep both sides happy -- let's keep topic of whether to use it elsewhere).

  • @grahamc I think this PR doesn't enforce a policy by itself because there are also non-political benefits here apart from just having no PulseAudio code in the system (see SDL and closure size cases above). Generally w.r.t. PulseAudio hardware.pulseaudio.enable setting shows our preferences but it's not changed -- we don't have PulseAudio enabled by default and enable it for some DEs.
  • @oxij Do I understand correctly the usual behavior of software you've tested was to fall back to ALSA when PulseAudio is not available? Does it try to start it by itself if not started when using libpulseaudio? If not then PulseAudio's code isn't actually used much by applications when the daemon is not started and so the main benefits are closure size and sanity checks for applications, which I'm not sure worth LD_LIBRARY_PATH (see below);
  • The PR itself won't work with multilib applications (easily fixed). This would also break FHS environments because they don't inherit host's LD_LIBRARY_PATH (for a good reason -- filesystem hierarchy is totally different, it's dangerous to do). Finally (and most seriously, I'm not sure we can fix that at all for this PR) this would break PulseAudio for non-NixOS (just Nix) use cases.

@7c6f434c
Copy link
Member

I wonder how much the closure size benefit is actually rebuild frequency benefit — it is hard to meaningfully reduce the closure size of LibreOffice, Chromium or Wine, but reducing their rebuild frequency could be more feasible (and PA separation probably does remove some of the dependencies even from Chromium)

@grahamc
Copy link
Member

grahamc commented Mar 13, 2018

This isn't about enforcing policy, but this is setting a policy. The default configuration expresses default policies and positions of the Nixpkgs community and group:

  • unfree software being disabled by default is a policy that signals we don't care for unfree software, but aren't hard-lined against it.
  • almost all the services disabled by default is a policy that we believe in a fairly minimal base

Changing libpulseaudio to not libpulseaudio by default is setting policy. You can see this attempt to set this policy over and over again, as it is @oxij's favorite axe to grind. We have literally been accused of being part of the Stasi and have had some PulseAudio-related PRs compared to the systematic extermination of Jewish people.

  • users that don't run PulseAudio daemon and don't want to even
    link against libpulseaudio-vanilla for ethical, religious,
    and/or security reasons don't need to make any overrides and
    benefit from hydra cache,

By the same token of not enforcing policy, users that don't run PulseAudio daemon and don't want to even link against libpulseaudio-vanilla for ethical, religious, and/or security reasons can simply add the following to their packageOverrides: libpulseaudio = libcardiacarrest.

To be clear, I'm a strong 👎 on this change and similar attempts to change our currently PulseAudio-supportive policy without an explicit move by the community to accept this as a goal.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@grahamc
Copy link
Member

grahamc commented Mar 13, 2018

To be clear, I'm a strong -1 on this change and similar attempts to change our currently PulseAudio-supportive policy without an explicit move by the community to accept this as a goal.

@@ -228,6 +228,9 @@ in {
source = writeText "libao.conf" "default_driver=pulse"; }
];

environment.sessionVariables.LD_LIBRARY_PATH =
[ "${getLib overriddenPackage}/lib" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using LD_LIBRARY_PATH is a good idea for achieving this. In systemd jobs and when the environment gets cleared/filtered (like with sudo), LD_LIBRARY_PATH won't be inherited and has to be set manually.

Have you explored a solution like mesa-drivers in /run/opengl-drivers? The problem with that approach is that NixOS will be needed for PA-based applications. Maybe the RUNPATH can be patched so they will fall back to libcardiacarrest?

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@abbradar
Copy link
Member

abbradar commented Mar 13, 2018

@grahamc Hm, I see your point -- my understanding was only correct given that PulseAudio isn't started by apps when it's not available and this isn't the case as @oxij pointed out so it's a change of behavior. I don't have a strong opinion on whether we want this behavior (I'd probably not and this seems counter-intuitive to me but someone may rely on it).

@oxij

Do you mean I need to put /lib32 in there somewhere?

No, rather you'll need to put pkgsi686Linux.libpulseaudio into LD_LIBRARY_PATH too if say hardware.pulseaudio.support32Bit is enabled.

For them you can simply shove libs to where they belong in FHS environments.

The problem is that whether to enable PulseAudio is a system-wide decision and FHS environment is a package so we'd need new config flag "really enable PulseAudio" or something similar which NixOS module would then enable -- I don't like this personally but it can be done.

Well, not exactly. It does need some env hackery, though. Correct.

Not only env hackery -- we don't have a good way to put libraries from Nixpkgs into users' LD_LIBRARY_PATH and we don't always want this too. Of course LD_LIBRARY_PATH=/usr/lib is also not an option. #9415 is the same issue about libGL which would probably only go away when we switch to libglvnd. The best solution we have until now is special runners like https://github.com/guibou/nixGL which would really worsen experience for PulseAudio+non-Nix users.

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@abbradar
Copy link
Member

Oops, sorry, I misread that part as being a question about libpulseaudio, not about the application. But the rest is still valid.

Just to be clear -- when application uses libpulse (or libpulse-simple) and tries to open a connection to it with some default settings, would those libraries try to launch PulseAudio? Not via DBus service launching as this requires a PulseAudio .service files installed so that's not applicable when services.pulseaudio.enable is disabled. I ask because if no this PR doesn't actually change any defaults and doesn't add security hardening much too -- with PulseAudio disabled applications won't use and almost code from it anyway. If yes on the other hand it does constitute a change of default behavior (okay by me but may be difficult if something relies on it).

libglvnd

I agree that it was made for a different reason, what I meant is that our current usage of LD_LIBRARY_PATH is exactly for those cases -- to give applications machine-specific drivers (libGL, sane and others), and we'd like to move away from it when we can because there are multiple issues with it. BTW some applications also load dynamic libraries from hard-coded /run/current-system/sw/lib path (like Qt plugins). I wanted to consolidate all of this into one /run/dynamic-libraries{,-32}; PR was somewhere but it's unfinished (maybe I'll continue one day).

, if you think FHS problem is too much of inconvenience, ok, I see your point.

FHS was just an example of problems with LD_LIBRARY_PATH so don't think over it much -- it's very similar to non-NixOS case so if it's solved probably FHS issue would be resolved automatically.

libpulsevnd

May be the solution yes -- I'm not sure if that much work is worth it but technically that should solve what I pointed out. I hope there's something better though -- personally I feel it's too much complexity. Before implementing anything serious however we'd need to find out if it's a behavior change in the end and if it is, should we do it or not.

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@abbradar
Copy link
Member

Ah, you have answered my first question simultaneously with my post ^^.

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@edolstra
Copy link
Member

@oxij Please cut it out with the constant attacks on and belittling of other contributors. (E.g. asking them if they're Stasi. We're talking about a sound system here, FFS.) The tone of smug superiority is also getting a bit tiresome.

BTW I have no strong feelings about PA, to be honest ALSA worked well enough for me for years. But it makes sense to standardize on one sound system, just as we standardize on a C library, init system, kernel, etc. That doesn't have to be PA necessarily, but my relatively uninformed impression is that PA is currently the most widely supported by applications.

@7c6f434c
Copy link
Member

7c6f434c commented Mar 13, 2018

@edolstra technically speaking, if NixOS isn't going to select a One True WM, the approach offered doesn't change the status of PulseAudio API that applications support, it can just be routed through an actual daemon or via apulse or to bitbucket.

@edolstra
Copy link
Member

@7c6f434c It changes the status on non-NixOS systems, though, since they won't have the real pulseaudio in LD_LIBRARY_PATH.

@domenkozar
Copy link
Member

I consider all of the pros rather philosophical than pragmatic, let me argue:

Smaller closure size when not using PulseAudio, much smaller closure size when not running either of PulseAudio, SystemD.

2.0M    /nix/store/7h0knqy0dix89i9i1zf2k6yybn5s4rjz-libpulseaudio-10.0

Relative size to the closure size of any running system I'd start elsewhere if closure size was to be improved. 2MB is marginally small.

You can't statically link against libcardiacarrest by accident, unlike the vanilla libpulseaudio (SDL did this before, see #36377).

That code had "--disable-pulseaudio-shared" - I wouldn't consider this an accident. The code instructed to do so.

Hence, libpulseaudio is no longer an ethical and security liability (see https://github.com/oxij/libcardiacarrest/blob/master/README.org for the case against libpulseaudio).

In that README I see lots of accusations, but no solid proofs about the software as unethical or unpatched.

Since it's a different implementation, it makes some buggy software crash sometimes. Which is good.

We try to package software so it doesn't crash, while I see benefit in crashing software as a QA, that's not the desired outcome of our work as Linux distribution.

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@oxij
Copy link
Member Author

oxij commented Mar 13, 2018 via email

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018 via email

@oxij oxij mentioned this pull request Mar 14, 2018
8 tasks
@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

Unless I'm missing something, that would also fix #9415 and remove the need for https://github.com/guibou/nixGL.

@lukateras
Copy link
Member

lukateras commented Mar 14, 2018

Sound servers are better for applications than directly consuming APIs of the underlying platform. It allows much more flexibility in sound output. I think we should embrace PulseAudio, allow Jack for professional real-time sound I/O, and deprecate direct use of ALSA.

Additionally, with an audio server, say PulseAudio, it is feasible to write a client in a scripting language and use it without any native code or even platform dependency (PulseAudio also runs on macOS, FreeBSD, OpenBSD, and Windows). This is a step in the right direction towards truly portable interactive applications.

OpenBSD also ships a sound server turned on by default, called sndiod.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

@yegortimoshenko Sound daemons and "directly consuming APIs of the underlying platform" are completely separate issues. The first is a process, the second is an API.

In your own example, you won't be able to implement PA client in pure Python or LISP because PA is very far from being anything like 9P (of Plan9) or OSS, you'd need at least some FFI library for it (because just look at the actual PA API). And, I argue, libpulse is the worst possible such library. Literally anything else (be it libao or openal) is better (smaller, saner API, better code quality). They also work on anything, including Windows. And don't need a sound daemon for it.

OpenBSD ships with sndiod because they don't have a library that can do dmix. (Also, I would think, because they have a different security model for sndio as compared to /dev/dsp of ALSA). In Linux terms sndiod is, basically, jackd (v1, not v2). Even the list of features is pretty much the same.

@lukateras
Copy link
Member

In your own example, you won't be able to implement PA client in pure Python or LISP [...]

https://github.com/mscdex/paclient

@lukateras
Copy link
Member

lukateras commented Mar 14, 2018

You are right in that openal API is certainly better, say, for games. I should not have emphasized this point: the main one is that sound servers are more flexible than ALSA and provide better abstraction layer that is useful in many situations.

@Ericson2314
Copy link
Member

NixOS/nix#1080 + something like tbd files on darwin means we can build against library interfaces not libraries and then automatically reuse the intensional store path rewriter to link any dynamically-linked library implementation we want. Yes, that's a huge amount of work away, but might be the only non-political way to resolve these sorts of things.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

Wow, okay, I take that point back. I didn't think anyone would be crazy enough to actually parse and generate PA socket protocol messages (and I'm pretty sure their format is unstable, but small subset would probably work). https://github.com/mscdex/paclient/blob/master/lib/client.js gives me chills, but I agree that this is better than the code in the vanilla libpulse. Let's all switch to that instead.

EDIT: The last sentence is a joke.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

@yegortimoshenko

provide better abstraction layer that is useful in many situations

What situations?

You see, I find it strange that "the common user" supposedly wants pure DRI (Wayland, which is a library) instead of X11 (which is a daemon) and, at the same time, a sound daemon (be it PA, jack or sndiod) instead of some common library API.

I agree that both sound daemons and X11 have their uses, but why would you want any of them on a normal laptop I have no idea.

@lukateras
Copy link
Member

lukateras commented Mar 14, 2018

I actually like X11 (good parts, including being a server) and PulseAudio more than Wayland and ALSA, so it's consistent. I'm quite sure people like Wayland because it's new, not because it's not a server.

Anyway, it is beyond the point: the actual problem here (I think) is that you want to replace libpulse implementation with one that talks with ALSA directly, and that is not possible without a mass rebuild, unless you own a build server.

In that case, why not just add libcardiacarrest to LD_LIBRARY_PATH? Original libpulse will still be in the store, but apps that support ALSA as a fallback will use it directly.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

@yegortimoshenko

I actually like X11

Well, okay, then. Let's agree to disagree on that point.

the actual problem here (I think) is that you want to replace libpulse implementation with one that talks with ALSA directly

libcardiacarrest doesn't talk to ALSA either, it only liberates you from the requirement to talk to PulseAudio. See the first sentence in the OP.

I do own a build farm and I did rebuild most of Nixpkgs with this change without issues already. Most of the discussion above is politics.

@lukateras
Copy link
Member

libcardiacarrest doesn't talk to ALSA either, it only liberates you from the requirement to talk to PulseAudio. See the first sentence in the OP.

I understand that, but it results in the app using ALSA directly.

What I mean is, your diff adds libpulse to environment.sessionVariables.LD_LIBRARY_PATH. Why not just do it vice versa and add libcardiacarrest there, if you don't want to use libpulse?

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

In that case, why not just add libcardiacarrest to LD_LIBRARY_PATH? Original libpulse will still be in the store, but apps that support ALSA as a fallback will use it directly.
What I mean is, your diff adds libpulse to environment.sessionVariables.LD_LIBRARY_PATH. Why not just do it vice versa and add libcardiacarrest there, if you don't want to use libpulse?

You can do that too (and in the README linked above I recommend doing exactly that for firefox if you can, for security reasons), but then you won't get any of the other benefits from the OP and discussed in the rest of this thread.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

@Ericson2314 if I understand those issues correctly, they have a much more profound effect than my #36881 (comment). And I'm not sure how you plan to implement all of that on top of current nix and ld-linux. But it would be nice to be able to do something like that, indeed.

@oxij
Copy link
Member Author

oxij commented Mar 14, 2018

@Ericson2314 Also, I think that if what you actually want is fast rebuilds (for security and stuff), you don't actually need most of that is discussed in that issue. Proper ccache stdenv (or, harder, nix-build build phases caching) would be able to achieve very fast rebuild times with minimal purity loses too. And super-hot fixes can be made with LD_LIBRARY_PATH/RPATH overrides.

@Ericson2314
Copy link
Member

@oxij but with the things I mentioned, mass rebuilds either go away or can be speculatively done in complete parallel as long as symbols aren't mass-changed. Absent exported functions in headers, a compiler ABI change, or some refactor moving other output files around, that won't be the case!

@Ericson2314
Copy link
Member

And that's 0 purity loss other than trusting the path renamer (which is only slightly more risky than the references detection we trust today).

@7c6f434c
Copy link
Member

Path renamer might be more risky than reference detection: the reference detection needs to find at least one instance of all the copies of a string, path renamer needs to catch them all…

@Ericson2314
Copy link
Member

Mmmm, good point @7c6f434c

@Ekleog
Copy link
Member

Ekleog commented Jan 21, 2019

Given that:

  • The discussion appears to be basically over
  • This PR has, given previous discussion and controversy, currently 0 chances of getting merged without going through an RFC

I am going to close this PR.

Let us not construe this as a statement either for or against the idea or code raised here. However, I do think that such a change should either not break anything in any case compared to the current situation (which, from my reading of the discussion, is not currently the case), or that it should go through an RFC.

And given that @grahamc appears to be strongly against the change, maybe it should go through an RFC even if it does not break anything. The new RFC process should hopefully make it hard for a RFC to get lost, so the situation is not like before it with all RFCs falling into oblivion.

@Ekleog Ekleog closed this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+
Projects
None yet
Development

Successfully merging this pull request may close these issues.