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/alsa: kill sound.enable and friends with fire #326262

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jul 11, 2024

Description of changes

This is an alternative take on #319839, which just removes most of sound.enable and provides instructions to mitigate the impact for people that actually use it for things.

Testing on JACK and Pulse setups would be very much appreciated. All the affected tests still work , except sfxr-qt which segfaults for seemingly unrelated reasons and pt2-clone which does not actually build (CC @fgaz for both of those).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@fgaz
Copy link
Member

fgaz commented Jul 11, 2024

pt2-clone: #326267

@fgaz
Copy link
Member

fgaz commented Jul 11, 2024

nix build github:K900/nixpkgs/no-more-sound-enable#nixosTests.sfxr-qt succeeds on my machine

@K900
Copy link
Contributor Author

K900 commented Jul 11, 2024

Then it's just flaky I guess? Fun.

@K900 K900 requested a review from eclairevoyant July 11, 2024 09:33
@Atemu
Copy link
Member

Atemu commented Jul 11, 2024

Hm, now we'll have to find the two people who still use ALSA to review this...

@K900
Copy link
Contributor Author

K900 commented Jul 11, 2024

I'm more worried about the JACK and Pulse setups, really. Actually, @cillianderoiste @lovek323 can you please look over the changes to your modules?

@K900
Copy link
Contributor Author

K900 commented Jul 13, 2024

I'm just merging this before there's more merge conflicts.

@K900 K900 merged commit 01fe231 into NixOS:master Jul 13, 2024
9 of 10 checks passed
@futile
Copy link
Contributor

futile commented Jul 13, 2024

Thanks a lot! Probably also makes sense to post a heads-up to https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574?

@K900
Copy link
Contributor Author

K900 commented Jul 13, 2024

I would hope that the manual changes cover this.

@futile
Copy link
Contributor

futile commented Jul 13, 2024

I would hope that the manual changes cover this.

I'll just post there then, and point people to the manual, if that's ok :) I always appreciate the heads-up from that thread.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/55

zimbatm pushed a commit to nix-community/srvos that referenced this pull request Jul 14, 2024
NixOS/nixpkgs#326262 made defining sound.enable an eval error.
@@ -3,132 +3,38 @@

with lib;

let

inherit (pkgs) alsa-utils;
Copy link
Member

@slotThe slotThe Jul 15, 2024

Choose a reason for hiding this comment

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

@K900 Speaking as one of the two people still running ALSA, setting hardware.alsa.enablePersistence to true gives me an undefined variable 'alsa-utils' error upon evaluation—I suppose because this line was removed? Or am I doing something wrong?

(EDIT: I should note that the error does not go away if I add alsa-utils to my systemPackages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry. Please test #327290

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 15, 2024

I'm really not happy about this change: why did you have to completely remove the module? A simple rename to "hardware.alsa" would have been enough to clear any possible confusion. The migration instructions are basically "you can reimplement it in your own configuration". By this logic NixOS could just drop every niche module, afterall you can just do it yourself, no?

Also, you removed alsa-utils from the environment, which is another undocumented breaking change.

@K900
Copy link
Contributor Author

K900 commented Jul 15, 2024

We have to be at least somewhat realistic about what we support and what options we provide. This module goes back like 15 years, which was a very different time, and just renaming it would not make people remove it from their configurations (as they should, generally), or make the actkbd mess work with modern setups.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 15, 2024

(as they should, generally)

Forget about the actkbd which has very limited use. Why should people be removing this module from their configuration?
If you use ALSA you would like to have a single toggle to install alsa-utils, setup the udev rules to restore the card settings and maybe even a toggle for the OSS compatibility (it's useful for BSD programs or old games).

@K900
Copy link
Contributor Author

K900 commented Jul 15, 2024

People should be removing this because all of this messes with Pulseaudio/Pipewire/JACK/whatever other sound daemon you're using (and you should be using one).

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 15, 2024

You can use ALSA without a sound demon and have the basic functionality of Pulseaudio/Pipewire/etc (per-application volume controls, software mixing, bluetooth audio...). Removing this module is really a pity because ALSA is basically the only sound system that can be extensively configured in a declarative way.

@K900
Copy link
Contributor Author

K900 commented Jul 15, 2024

Pipewire can absolutely be configured declaratively, including policy? I don't think Pulse can have declarative policy, but even that can have declarative configuration.

Xaver106 added a commit to Xaver106/NixOS-Config that referenced this pull request Jul 16, 2024
jtojnar added a commit to jtojnar/nixfiles that referenced this pull request Jul 18, 2024
Flake lock file updates:

• Updated input 'home-manager':
    'github:nix-community/home-manager/90ae324e2c56af10f20549ab72014804a3064c7f' (2024-07-11)
  → 'github:nix-community/home-manager/afd2021bedff2de92dfce0e257a3d03ae65c603d' (2024-07-16)
• Updated input 'nixpkgs':
    'github:nixos/nixpkgs/7e7c39ea35c5cdd002cd4588b03a3fb9ece6fad9' (2024-07-12)
  → 'github:nixos/nixpkgs/ad0b5eed1b6031efaed382844806550c3dcb4206' (2024-07-16)

  - Removes `sound.enable`: NixOS/nixpkgs#326262
  - Deprecates `i18n.inputMethod.enabled`: NixOS/nixpkgs#310708
TooDistractedToFocus pushed a commit to TooDistractedToFocus/nixos that referenced this pull request Jul 23, 2024
This option was removed upstream in NixOS/nixpkgs#326262
adrian-gierakowski pushed a commit to rhinofi/srvos that referenced this pull request Aug 15, 2024
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.

7 participants