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/wireplumber: add extraConfig / extraScripts options for WirePlumber 0.5 #292115

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

hcsch
Copy link
Contributor

@hcsch hcsch commented Feb 28, 2024

Description of changes

Follow up to #282377 and #291946. #282377 broke the use of environment.etc."wireplumber<...>" and did not yet add an extraConfig option to make up for that, so currently WirePlumber can only be configured via services.pipewire.wireplumber.configPackages. This is hardly ergonmomic, so this PR adds extraConfig / extraScripts options to fix that.

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.05 Release Notes (or backporting 23.05 and 23.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 28, 2024
@hcsch
Copy link
Contributor Author

hcsch commented Feb 28, 2024

@K900 I've created this PR now to try and fix the lack of an extraConfig option for WirePlumber. I'm not quite sure about some details yet though:

Do you know if WirePlumber .conf files (pre 0.5) can be split into multiple files in a conf.d of the same name? I couldn't find anything about that and in some very basic tests that didn't seem to be the case. If you can't split up .conf files in 0.4 I'll have to patch together a single config file from strings to link into the configFiles env. Not sure how duplicate definitions in .conf files work for 0.4 either though and I haven't gotten around to testing that. If duplicate definitions don't work or conf files don't support merging of objects, then users would have to copy the whole (or parts of the) default .conf file into their config, not just what they want to change.

All in all, this is not looking quite that trivial if extraConfig should work well and allow configuration of every aspect of WirePlumber. Perhaps reverting would be better until 0.5 lands after all...
I'm quite honestly a bit stressed out by how much breakage my changes have caused and want to get further changes right

@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 Feb 28, 2024
@hcsch
Copy link
Contributor Author

hcsch commented Feb 29, 2024

From a bit of testing I've done and from what I can tell based on https://gitlab.freedesktop.org/pipewire/wireplumber/-/blob/0.4/src/config, the top-level, non-lua .conf files for single-instance / multi-instance mode can't have fragments merged into them from the respective .conf.d in 0.4. Just copying the default contents and append some attributes (even if they are then duplicated in the file) works though. From how wireplumber behaved when I tested this (with the context.properties > log.level attribute) it seems that the latest definition of an attribute is what is used. Since you have to redefine top-level attributes though, you'll inadvertently unset some defaults if you don't copy those defaults over, even if you only want to specify one sub-attribute. So while people will be able to add config lines to .conf files, they will likely have to copy over some defaults as far as I can tell right now.

From a cursory search it also seems that nixpkgs doesn't currently (explicitly) support the multi-instance mode of wireplumber, right? I've added options for the config files relevant for that as well, just in case linking to the appropriate documentation. If you think it'd be less confusing to just not have those options, please do tell.

@hcsch
Copy link
Contributor Author

hcsch commented Feb 29, 2024

A caveat with the extraConfig option is that due to how it will append to the default .conf files provided by the wireplumber package the options will take lib.types.lines as opposed to pkgs.formats.json.type. Otherwise the module would have to strip outer curly braces and the resulting .conf file would be a mix of the lax SPA JSON and strict JSON. That should probably work, but I'm not sure that is that good an approach. With the extraConfig options just being strings, users can also just copy parts of the contents of the default .conf files as opposed to having to rewrite the default they want to change a sub-attribute of in nix.

@hcsch hcsch changed the title nixos/wireplumber: add extraConfig / extraScript options nixos/wireplumber: add extraConfig / extraLuaConfig / extraScripts options Feb 29, 2024
@hcsch
Copy link
Contributor Author

hcsch commented Feb 29, 2024

51e50d7 should probably go into a separate PR. I'll strip that out into a separate one when cleaning up this PR.

@hcsch hcsch force-pushed the wireplumber-extra-config branch 3 times, most recently from dc08fb9 to 8c58a55 Compare February 29, 2024 13:35
@hcsch
Copy link
Contributor Author

hcsch commented Feb 29, 2024

I've tested the current state of this PR on my NixOS setup with nixpkgs on nixos-unstable. Appending to /etc/wireplumber/wireplumber.conf via extraConfig.wireplumber works, adding a lua config to /etc/wireplumber/main.lua.d/ via extraLuaConfig.main works, and so does adding a lua script to /etc/wireplumber/scripts/ via extraScripts.
I've also tested that wireplumber picks up log.level = 4 in wireplumber.conf configured with the extraConfig option (see example for the option).

@hcsch hcsch marked this pull request as ready for review February 29, 2024 13:58
@hcsch hcsch force-pushed the wireplumber-extra-config branch from 8c58a55 to efdcaba Compare March 1, 2024 06:33
{
matches = {
{
{ "application.name", "matches", "io.github.celluloid_player.Celluloid" },
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very cool example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one came easily and I'm rather happy with it as well. I'll try and see if I can find similarly good examples for the others as well

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've added an example to the bluetooth and policy Lua config each. I gotta note that I've not tested these examples. I might try to today or in the next few days, but especially the policy config seems a bit hard to test since I know of nothing that actually uses policy defined endpoints at the moment (in the usual desktop setup of wireplumber that I have). Then again, maybe I just need to look harder.

@hcsch hcsch force-pushed the wireplumber-extra-config branch from efdcaba to 8653d9a Compare March 1, 2024 07:07
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 1, 2024
@hcsch hcsch force-pushed the wireplumber-extra-config branch from 8653d9a to 8604f01 Compare March 1, 2024 07:52
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 1, 2024
hcsch added a commit to hcsch/nixpkgs that referenced this pull request Mar 4, 2024
Document the breaking change caused by NixOS#282377. Assertions for better
error messages for this breaking change were added in NixOS#291946. NixOS#292115
will add a better migration path for WirePlumber by adding `extraConfig`
style options that previously did not exist for WirePlumber.
@hcsch
Copy link
Contributor Author

hcsch commented Mar 4, 2024

I've created #293248 to document the breaking change as it is now. Once that lands I'll add a commit to this PR to update the release notes again with the options added in this PR as a migration option (currently #293248 adds a note stating that configPackages is the only way to configure WirePlumber until this PR lands).

@hcsch hcsch force-pushed the wireplumber-extra-config branch from 8604f01 to 9908598 Compare March 4, 2024 14:16
@K900
Copy link
Contributor

K900 commented Mar 4, 2024

Wireplumber 0.5 is RC1 now, and moved back to old config structure: #278760

Maybe we should update this PR to use 0.5 config already?

@hcsch
Copy link
Contributor Author

hcsch commented Mar 5, 2024

Probably a good idea since it can be one option less with 0.5. extraLuaConfig can just be rolled into extraConfig then after all.
Do you think 0.5 will become stable soon enough?

@hcsch hcsch marked this pull request as ready for review March 19, 2024 15:47
@hcsch hcsch requested review from K900 and SuperSandro2000 March 19, 2024 15:47
@hcsch
Copy link
Contributor Author

hcsch commented Mar 19, 2024

Oh and should I roll #293248 into this PR, or should I keep that separate?
(I'll also update the wiki again once this gets merged)

@K900
Copy link
Contributor

K900 commented Mar 19, 2024

No strong preference either way. I'll take a closer look at this tomorrow.

Copy link
Contributor

@K900 K900 left a comment

Choose a reason for hiding this comment

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

Overall very cool, can you promise the examples still work? :)

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 20, 2024
@sersorrel
Copy link
Contributor

There's now some proper migration documentation from wireplumber's side: https://pipewire.pages.freedesktop.org/wireplumber/daemon/configuration/migration.html

It seems from some brief experimentation that scripts in /etc/wireplumber/scripts aren't loaded any more, they have to be somewhere in $XDG_DATA_DIRS (I guess that's probably going to be /run/current-system/sw/share).

@eclairevoyant eclairevoyant added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 27, 2024
@camelpunch
Copy link
Contributor

This PR would definitely have saved me some time. I'm tempted to go further in a future PR by providing a nix-native configuration for the JSON config, like many other modules do.

@hcsch hcsch force-pushed the wireplumber-extra-config branch from f452138 to a8571b7 Compare March 28, 2024 17:16
@hcsch
Copy link
Contributor Author

hcsch commented Mar 28, 2024

Sorry for the lack of activity last week. I've rebased to the current master and have also adopted the style of inheriting functions from lib in the let ... in at the top according to #299299. I think I'll just remove the examples I can't get to work / verify (Celluloid default volume and virtual-items). They can be added again later if/when somebody gets them to work. I'll add a short note about the dotted attribute names and how to write them in nix expressions to the description and then this should be good to go.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 28, 2024
@hcsch
Copy link
Contributor Author

hcsch commented Mar 28, 2024

How do you think that extraScripts should be best added? I'm currently thinking of adding them to XDG_DATA_DIRS in the environment for the wireplumber service. That way they should be able to override existing script files provided by wireplumber. So something like /nix/store/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-extra-scripts/share:/run/current-system/sw/share as XDG_DATA_DIRS for the service.

@hcsch
Copy link
Contributor Author

hcsch commented Mar 28, 2024

Then again. It'd be nicer for users if you could just take a peek at /run/current-system/sw/share/wireplumber/scirpts and everything relevant was in there (except for user local changes), I imagine. And /run/current-system/sw/share should be in XDG_DATA_DIRS by default anyways. Not quite sure how share/wireplumber currently ends up in /run/current-system/sw or how to add files to the that are preferred over those in the wireplumber package. Do I just chuck a package with them in environment.systemPackages with lib.mkAfter or lib.mkOrder 1200 or something like that?

@hcsch hcsch force-pushed the wireplumber-extra-config branch from f54bfd2 to d25999f Compare March 28, 2024 23:10
@hcsch
Copy link
Contributor Author

hcsch commented Mar 29, 2024

I've changed the implementation of extraConfig, extraScripts, and configPackages to now use the generated config env package in the XDG_DATA_DIRS variable of the WirePlumber service. This way environment.etc isn't used at all. Putting distro provided configuration / script files into XDG_DATA_DIRS also seems to be the currently recommended approach. I'm not sure if this would have been possible with WirePlumber 0.4, but this way we should cause almost no breakage for people upgrading from 23.11 to 24.05 (beyond that caused by the configuration format change).

I've tested that configuration is picked up by WirePlumber with the LDAC quality example form extraConfig. Scripts are picked up as well, which I tested with the following component and script definitions:

{
  services.pipewire.wireplumber = {
    extraConfig = {
      "test-script-component" = {
        "wireplumber.components" = [{
          name = "test-script.lua";
          type = "script/lua";
          provides = "custom.test-script";
        }];
        "wireplumber.profiles".main."custom.test-script" = "required";
      };
    };
    extraScripts = {
      "test-script" = ''
        print("Hello, world!")
      '';
    };
  };
}

This successfully prints "Hello, world!" in the WirePlumber log.

I still have to tweak the implementation a bit so that scripts can not only be directly in scripts/, but also in subdirectories, and then this should ready for real this time. I'll squash the commits at that point.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 29, 2024
@K900
Copy link
Contributor

K900 commented Mar 30, 2024

celestefox added a commit to celestefox/nixfiles that referenced this pull request Apr 20, 2024
transition kw-nixfiles -> tree
remove other uses of it
amd_pstate no longer a module
pipewire/wireplumber config update (revisit if/when NixOS/nixpkgs#292115 hits unstable?)
pinentry changes
neovim config fix
gaymes: rip yuzu
knot-resolver: disabled so I can build amaterasu for now, nixpkgs/nixos#301747 is almost to nixos-unstable and I can update again then)
@hcsch
Copy link
Contributor Author

hcsch commented Apr 20, 2024

Finally got around to finishing this. extraScripts now supports putting scripts in any relative path one wishes below share/wireplumber/scripts. I've tested both extraConfig and extraScripts with the example given in extraScripts. WirePlumber successfully prints "Hello, world!". I sadly don't really have a better example script, but I hope this is enough to at least show people the syntax for how to add scripts.
extraConfig and extraScripts now get grouped into their own packages built with pkgs.buildEnv. This way you can easily tell where config/script files are coming from with a tool like nix-tree (see image below showing the deps of unit-wireplumber.service in user-units < etc).

This is ready for review again now, and I'll squash the commits after that for merging.

Thanks for your help and patience so far :)

image

@hcsch hcsch requested a review from K900 April 20, 2024 20:07
Copy link
Contributor

@K900 K900 left a comment

Choose a reason for hiding this comment

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

SGTM. Squash?

Follow-up to NixOS#282377. NixOS#282377 broke `environment.etc."wireplumber<...>"`,
however WirePlumber did not yet have `extraConfig` style options for
configuring it ergonomically outside of `environment.etc`. This has
caused issues for people who had custom config files for WirePlumber, as
having to create a config package just to edit some settings is not as
ergonomic or discoverable as with a proper `extraConfig` style option.

This commit fixes this issue by adding the `extraConfig` option for
additional config file and the `extraScripts` option for additional
scripts to be used by config files.

With WirePlumber 0.5 it is possible to supply config files and scripts
via the `XDG_DATA_DIRS` variable to the WirePlumber daemon. This is how
the new options and with this change also the `configPackages` option
expose their files to the daemon. This way
`environment.etc."wireplumber"` works again for user configuration and
breakage of old configs from 23.11 to 24.05 should be limited to those
caused by the change in the config format from WirePlumber 0.4 to 0.5.
@hcsch hcsch force-pushed the wireplumber-extra-config branch from d2341a6 to 72ed337 Compare April 21, 2024 18:40
@hcsch
Copy link
Contributor Author

hcsch commented Apr 25, 2024

I've squashed the commits. If you have any more suggestions for what to test before getting this merged, feel free to tell me and I'll try my best to give those a try.

@K900 K900 merged commit 8596068 into NixOS:master Apr 29, 2024
21 checks passed
@hcsch hcsch deleted the wireplumber-extra-config branch April 29, 2024 12:37
MattSturgeon added a commit to MattSturgeon/nix-config that referenced this pull request May 20, 2024
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 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