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

tree-wide: no pulseaudio by default #35041

Closed
wants to merge 2 commits into from

Conversation

oxij
Copy link
Member

@oxij oxij commented Feb 16, 2018

Motivation for this change

I run with config.pulseaudio = false for years, it works perfectly.
But having it enabled by default in some packages and disabled in others in nixpkgs sends a mixed message: many people think that you need this broken piece of insecure bloatware to have sound, which is as far from being true as humanly possible (see #29250).

Lets just disable it by default so that "its already in the closure, so lets add it proper" (see #24012, #35005) motivation goes away.
Also lets just disable it by default to minimize the attack surface it provides.

If you think you need it by default go read #29250. If you think its ok to have it by default go read #29250.

If you do use it, the second patch to nixos now enables it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Tested execution of many binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@gebner
Copy link
Member

gebner commented Feb 16, 2018

I am strongly against this change. This PR does not even make it easier to run a system without pulseaudio running. It just causes lots of unnecessary rebuilds if you want to use pulseaudio.

I think that we should build packages with all optional dependencies included whenever possible so that it is possible to use the binary cache. In the case of pulseaudio, there is not even a big change in closure size.

@lheckemann
Copy link
Member

+1 @gebner. Build-time variability is something we want to avoid since it results in either (a) end users having to build loads of stuff locally or (b) combinatorial explosions of what hydra has to build. This is why nixpkgs.config is avoided in most of nixpkgs. Applications built against pulseaudio can still run without pulseaudio if they have alsa support. If they don't have alsa support they weren't going to produce sound without pulse anyway.

@lheckemann
Copy link
Member

Also since this causes a mass rebuild (over 500 packages affected on linux) this should be targetted to staging, not master

@oxij
Copy link
Member Author

oxij commented Feb 16, 2018

This PR does not even make it easier to run a system without pulseaudio running.

Please elaborate. Running without pulseaudio is the default. What can be easier?

In the case of pulseaudio, there is not even a big change in closure size.

Well, maybe not in NixOS, but in SLNOS there is a big change, because pulseaudio libs require systemd, dbus and deps.

rebuilds

It is true that if you want to have pulseaudio you will have to rebuild some of packages yourself after this PR. I think this is a good thing, as it makes it harder to shoot yourself in the foot.

@lheckemann
Copy link
Member

Well, maybe not in NixOS, but in SLNOS there is a big change, because pulseaudio libs require systemd, dbus and deps.

Then I wholeheartedly encourage you to apply this change to SLNOS.

Note that this PR in its current form will not remove libpulseaudio from that many closures (so not so much a "big change"), as it only covers a small portion of the uses of libpulseaudio. If you're intent on getting rid of it properly, you'll want to either remove it completely or find all other uses and make those conditional on the config too.

It is true that if you want to have pulseaudio you will have to rebuild some of packages yourself after this PR. I think this is a good thing, as it makes it harder to shoot yourself in the foot.

NixOS is not such an opinionated distro IME.

@oxij
Copy link
Member Author

oxij commented Feb 16, 2018

Right, the higher-tier SLNOS branch does exactly this (SLNOS is separated into tiers, from very NixOS-like experience where most of hydra cache still helps, to the very suckless system where you have to rebuild everything yourself).

To be honest, in the current state of things I wouldn't merge this PR to NixOS myself (I made it to make a point that it is that easy to get 90% there with only trivial changes). What I would do is I would still disable libpulseaudio for all the leaf packages, but leave it enabled by default for libraries like libao and ffmpeg. This seems like a okay middle ground suitable for NixOS to me. We can discuss the finer points of this if anyone cares.

@gebner
Copy link
Member

gebner commented Feb 16, 2018

Running without pulseaudio is the default.

Pulseaudio is already disabled by default, this PR changes nothing in this regard.

[...] disable libpulseaudio for all the leaf packages, but leave it enabled by default for libraries like libao and ffmpeg. This seems like a okay middle ground suitable for NixOS to me.

This is not acceptable since it causes unnecessary rebuilds, as discussed above. I think it's best to keep this change in SLNOS.

@oxij
Copy link
Member Author

oxij commented Feb 16, 2018 via email

Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

I'm against this because of issues already mentioned here (mainly build variance). We need a better solution like the one outlined in #35292 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants