wireplumber: init, add NixOS module#150548
wireplumber: init, add NixOS module#150548jtojnar merged 3 commits intoNixOS:masterfrom K900:wireplumber
Conversation
github-actions
bot
added
6.topic: nixos
|
Result of 1 package blacklisted:
1 package built:
|
ofborg
bot
added
8.has: package (new)
|
Good start. Maybe provide main/bluetooth/policy options to create a |
|
I was thinking about it, but I decided not to, because I honestly have no idea what people's configurations are going to look like (or why anyone would want to write a custom configuration in the first place). I'd give it a few months and see what people actually use to configure it... |
|
Agreed, just dropping the upstream default config in place should be a relatively safe starting point. If you don't mind, could you also change the pipewire module to have an enum for enabling one of the two session managers instead of separate enables for both? Ideally we wouldn't even have publicly visible enable options for the session managers. |
arcnmx
left a comment
arcnmx
left a comment
There was a problem hiding this comment.
Some notes in addition to the inline comments...
pkgs/development/libraries/pipewire/wireplumber.nixdoesn't quite seem like the right place to me, it is still a separate codebase from pipewire. It is however both a daemon and a library...- something along the lines of
outputs = [ "bin" "out" "dev" "doc" (and/or "man"?) ]is probably warranted here
There was a problem hiding this comment.
anything pkgs is used for should be explicitly added to this argument list instead
There was a problem hiding this comment.
there (probably?) should be an assertion to prevent mixing the two session managers
There was a problem hiding this comment.
imo it'd be nice if this were conditional on an enableDocs arg
There was a problem hiding this comment.
This likely needs to be optional on stdenv.hostPlatform == stdenv.buildPlatform due to longstanding issues wrt cross-compiling gobject-introspection.
There was a problem hiding this comment.
Added a separate enableGI argument that defaults to this, if someone really wants to shoot themselves in the foot, they can go ahead and override.
The upstream config is like 2000 lines of Lua, and it's not really intended to be modified, it's more likely you're going to write your own scripts on top of it. |
You're presumably talking about the upstream scripts? Those are very different from the upstream config, which despite being some 500 lines of lua is indeed very much intended to be modified by the user. It is not very fun to reproduce in nix. (essentially, the config lua scripts generate json(ish) configuration data to be passed to the scripts, which then actually do all the work - and lua is the only available method for providing said config data to the scripts. iow the lua configs serve as an overly complex preprocessor for the actual configuration data) |
I think the intent of that is that you write your own scripts that you drop in |
|
Also, added split outputs. |
|
Also, I haven't touched the NixOS side yet because I'm trying to think of a good way to make it extensible... Any ideas? |
Eh it's... a haphazard mess, really. If you copy the commented-out examples and override an existing file, you slowly diverge from upstream over time. If you only insert new files, you can pretend that you're not modifying config but any config code you write implicitly depends on the objects defined/used in the config files that come before and after your new config. But yes, insertion is meant to be possible as long as you never dare to use the env vars to do things like override/add scripts or configs, at which point the file resolution and merging from /etc to /share is unreliable and completely changes/breaks.
So I agree with what was said previously, that this is a fine start and module expansion can always happen later... but I'd probably suggest starting with having nix manage only the
|
|
So I came up with something kind of interesting. We can have |
|
Works On My Machine ™️ The lack of configuration should not be a problem for simple setups where everything just goes straight from/to the default sound card with an autodetected device profile, i.e. the vast majority of use cases... Although I'm not entirely sure how much overlap that crowd has with the NixOS userbase ;). Either way it's off by default so it shouldn't really be able to break any existing setups. Please add assertions to make |
|
My latest change will actually make it fail when both are enabled, as both will set |
|
Right. The error message even points to the right places so I guess that's fine. |
|
Whoops. Emergency hotfix. |
|
That said, it could still be more explicit about the |
|
Actually, we should probably just stop defaulting that to true, and instead default to wireplumber... |
|
Eventually yes since media-session is being phased out. But it's a bit too early for that, considering that you can't really configure wireplumber in the module's current state but configuration for media-session has been there for a long time and works fine. |
|
All right, let's stick to media-session for now then, and add the assertion. |
There was a problem hiding this comment.
Oh right, I believe the nixpkgs policy is to write the parameters one per line. Helps keep diffs a bit cleaner when adding / removing stuff in the future.
There was a problem hiding this comment.
Seems to be all over the place, but it makes sense. Added some newlines.
|
Cleaned up the commit history a bit, also switched |
|
Built & runs nicely on my desktop with that assertion fix and the latest pipewire release (#150999). |
|
Found one problem with wireplumber: it refuses to let me connect the pipewire filter-chain node running rnnoise to the capture input of Teams although it is supposed to be the default input. |
|
That sounds like an upstream issue maybe? |
|
Likely so, and it can probably be fixed with some configuration changes. Wireplumber overall seems to be unnecessarily eager to connect inputs to the front panel mic input that doesn't even have a mic plugged in. Anyway, seems pretty much to be expected at this stage and in a simpler setup it wouldn't be a problem anyway. |
|
Anything left to do here? I say we merge as-is and then I'll try to figure out the config story. |
jansol
left a comment
jansol
left a comment
There was a problem hiding this comment.
Yeah I think this is mergeable as a first step now.
|
Rebased to include latest pipewire systemwide changes from master, also updated to today's wireplumber 0.4.6. |
|
Will need to rerun the tests after #153658 |
There was a problem hiding this comment.
Is this option necessary? One can easily use overlays since this is not expected to be a dependency of any package so overlays should not cause any extra rebuilds.
There was a problem hiding this comment.
I'm not really familiar with using overlays, does that just automagically replace the corresponding package everywhere?
There was a problem hiding this comment.
Yeah, you can pass e.g. overlays = [ (final: prev: { wireplumber = prev.wireplumber.overrideAttrs (attrs: rec { version = "…"; src = "…"; }); }) ] to nixpkgs (or use nixpkgs.overlays option if you do not override the nixpkgs.pkgs option) and it will replace wireplumber in pkgs passed to the modules.
There was a problem hiding this comment.
Not really, but pipewire-media-session has it, so I thought it should be here for consistency as well. Also, wireplumber is (sort of) a library, so it's possible other things will start depending on it at some point.
There was a problem hiding this comment.
Is overlays > package options in modules a documented policy nowadays? If so, it makes sense to not have this option here at all and mark it deprecated in the pipewire module as well.
There was a problem hiding this comment.
It always felt to me that package options are cargo cult. They should only be necessary when overriding the package globally would cause rebuilds of the system (e.g. in case of systemd).
There was a problem hiding this comment.
This creates two different ways to set the session manager. Given that pipewire itself is not configured in any way by this, I think it might be cleanest to just install each session manager individually in its own module. This kind of de-duplication feels forced to me anyway.
There was a problem hiding this comment.
The idea was to have a working session manager by default and not require looking where it needs to be disabled when switching to a different one. Granted, it is a bit redundant if media-session is going to be removed as soon as wireplumber is usable.
There was a problem hiding this comment.
These conflict with each other, you can only assign systemd{.user}.services.${cfg.sessionManager.package.unitName} once so to set both it has to be done with an object rather than standalone attributes, i.e.:
systemd.services."${cfg.sessionManager.package.unitName}" = {
enable = if cfg.sessionManager.enable then cfg.systemWide else false;
wantedBy = [ "pipewire.service" ];
};
systemd.user.services."${cfg.sessionManager.package.unitName}" = {
enable = if cfg.sessionManager.enable then !cfg.systemWide else false;
wantedBy = [ "pipewire.service" ];
};
But as mentioned in another comment it's probably better to not do this whole dance at all and just get rid of service.pipewire.sessionManager completely. The only thing it gets us atm is it says that two places are trying to set the option, and there is an assertion to guard against that anyway.
There was a problem hiding this comment.
All right, let's keep it simple for now then.
jtojnar
left a comment
jtojnar
left a comment
There was a problem hiding this comment.
Just few more nits, looks great otherwise.
This is extremely basic for now, but we can add more stuff later
jansol
left a comment
jansol
left a comment
There was a problem hiding this comment.
LGTM & works on my setup.
6 participants
Motivation for this change
Upstream recommends this over pipewire-media-session: https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/41392eda7fb0e8b27a0167ef04241b18e9c14bf8/NEWS#L295
Depends on #153658
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes