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

Pipewire 0.3.67 + big module cleanup #220332

Merged
merged 4 commits into from
Mar 14, 2023
Merged

Pipewire 0.3.67 + big module cleanup #220332

merged 4 commits into from
Mar 14, 2023

Conversation

K900
Copy link
Contributor

@K900 K900 commented Mar 9, 2023

Description of changes
  • update pipewire to new upstream release
  • add tinycompress support
  • (POSSIBLY LARGE BREAKING CHANGE) remove extremely broken config overriding
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/)
  • 23.05 Release Notes (or backporting 22.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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 9, 2023
@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Mar 9, 2023
@ofborg ofborg bot requested review from jtojnar and Kranzes March 9, 2023 13:12
@@ -144,6 +144,9 @@ In addition to numerous new and upgraded packages, this release has the followin

- conntrack helper autodetection has been removed from kernels 6.0 and up upstream, and an assertion was added to ensure things don't silently stop working. Migrate your configuration to assign helpers explicitly or use an older LTS kernel branch as a temporary workaround.

- The `services.pipewire.config` options have been removed, as they have basically never worked correctly. All behavior defined by the default configuration can be overridden with drop-in files as necessary.
Copy link
Member

Choose a reason for hiding this comment

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

Where to place those drop-in files? That's a bit unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how verbose we want to be here. The drop-in files go into /etc/pipewire/<whatever>.conf.d and there are multiple config files, so it's hard to explain without getting into a lot of detail. Also I kinda just hope no one is using this, because it does not work the way anyone would expect.

Copy link
Member

Choose a reason for hiding this comment

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

Given that there's a whole wiki page dedicated to tweaking services.pipewire.config, I think going into detail is warranted.

Copy link
Member

Choose a reason for hiding this comment

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

You'd probably even want to announce it in the breaking changes thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That entire page will need to be updated. It's on my list. I'll probably write up a post detailing the migration steps and post that in the thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The migration guide should preferrably be mentioned in the release notes too, since it's a pretty big chunk of not necessarily obvious manual work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely going in the docs somewhere, I just need to find a good place to put it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up adding the migration guide to the end of the release notes.

@mweinelt mweinelt requested a review from jansol March 9, 2023 13:15
@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/19

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.

Looks good, should make future updates a lot simpler since we won't have to care about default config changes any more. I'd have to check what this implies for my filter-chain and custom node setups. I guess I'd just keep my existing setup for those since it was just writing custom files to /etc to begin with.

But it'll be at least one more week before I can get to that so it's probably not necessary to wait for me if there seems to be a consensus already.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@@ -144,6 +144,9 @@ In addition to numerous new and upgraded packages, this release has the followin

- conntrack helper autodetection has been removed from kernels 6.0 and up upstream, and an assertion was added to ensure things don't silently stop working. Migrate your configuration to assign helpers explicitly or use an older LTS kernel branch as a temporary workaround.

- The `services.pipewire.config` options have been removed, as they have basically never worked correctly. All behavior defined by the default configuration can be overridden with drop-in files as necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

The migration guide should preferrably be mentioned in the release notes too, since it's a pretty big chunk of not necessarily obvious manual work.

@K900 K900 force-pushed the pipewire-0.3.67 branch 2 times, most recently from 2982a63 to 61088d9 Compare March 10, 2023 10:30
@K900
Copy link
Contributor Author

K900 commented Mar 12, 2023

So, should I just merge this at this point? It looks like either no one is testing it or no one is having issues...

@mweinelt
Copy link
Member

I would expect that no one is testing. 😉

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

You would be wrong! I'm affected by a bug that's supposedly fixed in 0.3.67 and I'm testing this as we speak but the bug is hard to repro.

The NixOS changes LGTM. I can't really test pipewire itself much due to the amount of rebuilds but building the services up manually works just fine via pipewire-pulse and mpv.

@Atemu
Copy link
Member

Atemu commented Mar 12, 2023

(I haven't tried the new way of configuring modules but I didn't use the old way either and had stateful config files in ~/.conf which still work fine.)

@mweinelt mweinelt added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 13, 2023
K900 added 4 commits March 14, 2023 20:31
Will be used by Pipewire later.
- drop media-session (rip 💀)
- stop trying to let people override default configs, those never got merged correctly
- drop all the complexity arising from having to vendor default config files
- build docs in sandbox as we no longer recurse
@K900 K900 force-pushed the pipewire-0.3.67 branch from 61088d9 to 1fab869 Compare March 14, 2023 17:31
@K900
Copy link
Contributor Author

K900 commented Mar 14, 2023

I'm just going to send this to avoid any more merge conflicts.

@K900 K900 merged commit 2482552 into NixOS:staging Mar 14, 2023
@trofi
Copy link
Contributor

trofi commented Mar 22, 2023

I have the following config module that I fail to convert to a new way:

{ ... }:
{
  # rtkit is optional but recommended
  security.rtkit.enable = true;
  services.pipewire.enable = true;
  # emulation layers:
  services.pipewire.alsa.enable = true;
  services.pipewire.alsa.support32Bit = true;
  services.pipewire.pulse.enable = true;
  # allow other user use sound by absolute address:

  services.pipewire.config.pipewire-pulse = {
    "pulse.properties" = {
      "server.address" = [
        # default:
        "unix:native"
        # extension:
        "unix:/tmp/pulse-for-all"
      ];
    };
  };
  hardware.pulseaudio.extraClientConf = ''
    default-server=unix:/tmp/pulse-for-all
  '';
  # Also add an activation:
  # ~/.config/systemd/user/pipewire-pulse.socket.d:
  #   [Socket]
  #   ListenStream=/tmp/pulse-for-all
}

Specifically services.pipewire.config.pipewire-pulse now fails with:

       error:
       Failed assertions:
       - The option definition `services.pipewire.config' in `/etc/nixos/pipewire.nix' no longer has any effect; please remove it.
       Overriding default Pipewire configuration through NixOS options never worked correctly and is no longer supported.
       Please create drop-in files in /etc/pipewire/pipewire.conf.d/ to make the desired setting changes instead.

I tried a few override types via /etc/pipewire/pipewire.conf.d/ and all of them failed. Just adding something like

{
  "pulse.properties": {
    "server.address": [
      "unix:native",
      "unix:/tmp/pulse-for-all"
    ],
  },
}

does not work. Do I need to copy the full config file to recover from the breakage?

@K900
Copy link
Contributor Author

K900 commented Mar 22, 2023

Those settings go into pipewire-pulse.conf.d, not pipewire.conf.d

@trofi
Copy link
Contributor

trofi commented Mar 22, 2023

Thank you! I think I was misled by the warning message. Bellow config did the trick:

  environment.etc."pipewire/pipewire-pulse.conf.d/pulse-for-all.conf".text = ''
    pulse.properties = {
      # the addresses this server listens on
      server.address = [
        # default:
        "unix:native"
        # extension:
        "unix:/tmp/pulse-for-all"
      ]
    }
  '';

@Atemu
Copy link
Member

Atemu commented Mar 22, 2023

I feel like we should have a more abstract way of configuring that.

@K900
Copy link
Contributor Author

K900 commented Mar 22, 2023

What exactly?

@Atemu
Copy link
Member

Atemu commented Mar 23, 2023

Configuring PW, PW-pulse etc. "Put a file with some text into /etc" is about as low-level as it gets. That's documentation on the level I'd expect to find on the Arch Wiki, not in NixOS.

I'm thinking resurrecting the config options and simply toJSONing the values into files in pipewire.conf.d etc. in the module; an abstract interface.

@K900
Copy link
Contributor Author

K900 commented Mar 23, 2023

I agree with that, maybe naming it extraConfig or something, I just haven't gotten around to it yet.

@K900
Copy link
Contributor Author

K900 commented Mar 23, 2023

(feel free to submit a PR if that's something you want to work on, btw)

@Atemu
Copy link
Member

Atemu commented Mar 23, 2023

As per NixOS/rfcs#42, it should be settings. I'll see if I can work on that this weekend but I wouldn't bet on it.

@K900
Copy link
Contributor Author

K900 commented Mar 23, 2023

I'd rather have the option name indicate that it's being merged with the defaults somehow.

@Atemu
Copy link
Member

Atemu commented Mar 23, 2023

Prior art on how to handle that:

settings = mkOption {
description = lib.mdDoc ''
Attrset that is converted and passed as TOML config file.
For available params, see: <https://github.com/DNSCrypt/dnscrypt-proxy/blob/${pkgs.dnscrypt-proxy2.version}/dnscrypt-proxy/example-dnscrypt-proxy.toml>
'';
example = literalExpression ''
{
sources.public-resolvers = {
urls = [ "https://download.dnscrypt.info/resolvers-list/v2/public-resolvers.md" ];
cache_file = "public-resolvers.md";
minisign_key = "RWQf6LRCGA9i53mlYecO4IzT51TGPpvWucNSCh1CBM0QTaLn73Y7GFO3";
refresh_delay = 72;
};
}
'';
type = types.attrs;
default = {};
};
upstreamDefaults = mkOption {
description = lib.mdDoc ''
Whether to base the config declared in {option}`services.dnscrypt-proxy2.settings` on the upstream example config (<https://github.com/DNSCrypt/dnscrypt-proxy/blob/master/dnscrypt-proxy/example-dnscrypt-proxy.toml>)
Disable this if you want to declare your dnscrypt config from scratch.
'';
type = types.bool;
default = true;
};
(by yours truly)

fufexan added a commit to fufexan/nix-gaming that referenced this pull request Mar 27, 2023
fufexan added a commit to fufexan/nix-gaming that referenced this pull request Mar 28, 2023
fufexan added a commit to fufexan/nix-gaming that referenced this pull request Mar 28, 2023
fufexan added a commit to fufexan/nix-gaming that referenced this pull request Apr 3, 2023
fufexan added a commit to fufexan/nix-gaming that referenced this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants