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

nixos/pulseaudio: remove broken support for 32bit on 64bit systems #157352

Conversation

ivanbrennan
Copy link
Member

@ivanbrennan ivanbrennan commented Jan 30, 2022

Motivation for this change

In #154276, alsa-lib was updated and old patches removed. One of those patches, from 2014, allowed alsa-lib to recognize a libs field in /etc/asound.conf, and the pulseaudio module leveraged this to optionally add support for running both 32bit and 64bit apps on 64bit platforms.

With the patch removed, alsa-lib is unable to parse the /etc/asound.conf generated by the pulseaudio module (regardless whether the user has opted into 32bit support), and alsa utils such as alsactl, alsamixer, speaker-test, are broken. E.g.

$ alsactl monitor default
alsa-lib control.c:1464:(snd_ctl_open_conf) Unknown field libs
Cannot open ctl default

Once the issue was identified, it was reasoned that the 32bit support is too niche a use-case to justify patching a system as complex as alsa, especially given the availability of pipewire.

I removed the support32Bit option from pulseaudio and changed the generated config to use syntax recognized by alsa-lib. I then removed several references to this option from other modules (steam, jack, pipewire).

Note: I rebuilt my system against this branch and confirmed that alsa works as expected for my use-cases. I have not tested steam, jack, or pipewire.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Back in 2014, the following commits patched alsa-lib to support running
apps with 32bit sound on 64bit platforms, and configured
/etc/asound.conf to leverage that support:
ab8ef63
0c8ad65

A recent commit updated alsa-lib (1.2.5.1 -> 1.2.6.1), and removed the
patch, but left the pulseaudio module unchanged, meaning it continued to
generate an etc/asound.conf containing the `libs` field, which alsa-lib
cannot recognize (the patch was providing support for that field).
aeea1bb

As a result, a system whose configuration enables pulseaudio can no
longer run alsa utils, regardless of whether support32Bit is enabled.
E.g.

  $ alsactl monitor default
  alsa-lib control.c:1464:(snd_ctl_open_conf) Unknown field libs
  Cannot open ctl default

  $ speaker-test -t wav -c 2

  speaker-test 1.2.6

  Playback device is default
  Stream parameters are 48000Hz, S16_LE, 2 channels
  WAV file(s)
  ALSA lib pcm.c:2576:(snd_pcm_open_conf) Unknown field libs
  Playback open error: -22,Invalid argument

Once the issue was identified, it was reasoned that the 32bit support is
too niche a use-case to justify patching a system as complex as alsa,
especially in 2022 and with pipewire providing a viable alternative
solution.

Remove the unused (and now unusable) 32bit configuration code from
/etc/asound.conf, so alsa-lib can again parse it and run properly.
As described in the previous commit, this option was removed. Remove
references to it, and produce etc/asound.conf files that alsa-lib is
able to parse.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: steam Steam game store/launcher (store.steampowered.com) 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 30, 2022
@ivanbrennan
Copy link
Member Author

@L-as You mentioned,

The only scenario I can think of where this is still relevant is Steam with proprietary old 32-bit games that use ALSA directly, but you can just switch to PipeWire for that! Perhaps that's just something we should note in the release notes for the next NixOS release.

I'm not sure what action the notes should suggest to affected users. Could you advise?

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 30, 2022
@Riey
Copy link
Member

Riey commented Jan 30, 2022

I also have this issue in current master (not pulseaudio but pipewire/alsa.enable = true)

with this patch, I check speaker-test alsamixer and some steam games(Hollow Knight and Oxygen Not included) and works fine

@Synthetica9
Copy link
Member

... Now I'm confused about what hardware.pulseaudio.support32Bit did? I just rebased #153916 on master and commented out support32Bit but the 32 bit portion of the test still works...

@L-as
Copy link
Member

L-as commented Jan 30, 2022 via email

@mdarocha mdarocha mentioned this pull request Jan 30, 2022
5 tasks
Copy link
Contributor

@jansol jansol left a comment

Choose a reason for hiding this comment

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

The alsa 32bit options have been pretty widely recommended in the past due to steam so I'd like to see a proper deprecation error instead of just silently removing the options.

The pipewire portion of the diff looks good to me otherwise.

Use an assertion to display a proper error message if the user's config
has the `support32Bit` option enabled. Example error output:

  error:
         Failed assertions:
         - `hardware.pulseaudio.support32Bit` is no longer supported.
@@ -210,6 +218,11 @@ in {
}

(mkIf cfg.enable {
assertions = [{
assertion = !cfg.support32Bit;
message = "`hardware.pulseaudio.support32Bit` is no longer supported.";
Copy link
Member Author

Choose a reason for hiding this comment

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

The alsa 32bit options have been pretty widely recommended in the past due to steam so I'd like to see a proper deprecation error instead of just silently removing the options.

@jansol That makes sense. This commit adds a clearer message, but did you mean an actual deprecation process, where we fix the currently broken 32bit support and provide a deprecation warning about its eventual removal? That would require reintroducing the patch that was removed in #154276, but I'm not opposed to doing that if it's warranted (and it sounds like it is).

I'd also like to provide a more informative message than I've done in this commit, something like,

In order to support 32bit audio on a 64bit system, please use ??? instead.

But I don't know enough about this domain to do so. Is there a pipewire-related suggestion that would be appropriate in place of ??? above?
cc @L-as

@ivanbrennan ivanbrennan requested a review from jansol January 31, 2022 00:46
@ivanbrennan ivanbrennan force-pushed the pulseaudio-remove-32bit-asound-conf branch from 02c141e to 404138f Compare January 31, 2022 02:38
@starcraft66
Copy link
Member

Tested this branch on my system and it fixed pipewire support for ALSA within Mixxx.

@dtzWill
Copy link
Member

dtzWill commented Jan 31, 2022

Thanks for working on this!

Small note: this also drops (similarly broken) 32bit alsa support for jack and pipewire (removing the libs entries from those asoundrc's as well), perhaps update the PR title?

@ivanbrennan
Copy link
Member Author

Small note: this also drops (similarly broken) 32bit alsa support for jack and pipewire (removing the libs entries from those asoundrc's as well), perhaps update the PR title?

@dtzWill That's a good point, but I'm considering changing the direction of this PR to fix the 32bit support (re-adding the alsa-lib patch that was removed) and deprecate the option rather than removing it right away. (see #157352 (comment))

I'll update the PR title one way or the other though 👍

@ivanbrennan ivanbrennan marked this pull request as draft February 1, 2022 00:39
@ivanbrennan
Copy link
Member Author

Given that it's not clear what alternative to suggest to users who currently rely on the 32bit support, I think the best thing is to fix what's currently broken, and leave the deprecation/removal of 32bit support to someone knowledgeable enough to compose appropriate deprecation warnings (including suggested config changes for affected users).

So I've created an alternative PR that confines itself to fixing what's currently broken: #157631

@ivanbrennan
Copy link
Member Author

Closing this in favor of #157631

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 6.topic: steam Steam game store/launcher (store.steampowered.com) 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants