nixos/pulseaudio: make daemon.conf configurable #20888
Conversation
|
@sternenseemann, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rickynils, @Profpatsch and @peterhoeg to be potential reviewers. |
| ''; | ||
| }; | ||
|
|
||
| extraConfig = mkOption { |
There was a problem hiding this comment.
Instead of going through plaintext splicing, this should use an attribute set and apply a generator after applying all options.
#20903
There was a problem hiding this comment.
I suggest just having one config option instead of reproducing misc options of the daemon and reference to the manpage of the daemon for values & defaults. Or alternatively generate the config automatically from the sources. :P
There was a problem hiding this comment.
@Profpatsch, do you have an example of a module implementing this?
| @@ -160,6 +167,31 @@ in { | |||
| if activated. | |||
| ''; | |||
| }; | |||
|
|
|||
| resample-method = mkOption { | |||
| type = types.str; | |||
There was a problem hiding this comment.
It's nicer to use an enum - there are only 27 possible values.
There was a problem hiding this comment.
sounds like a lot, who will keep this list up-to-date?
There was a problem hiding this comment.
that’s why I propose just having a config and not copy items with potentially out-of-date defaults when upstream decides to change them.
There was a problem hiding this comment.
@peterhoeg I think it even might vary depending on different pulseaudio packages (and if it is just in the future).
There was a problem hiding this comment.
do you have an example of a module implementing this?
A simple example would be
{ options.hardware.pulseaudio.daemon.config = mkOption {
type = types.attrsOf types.unspecified;
default = {};
description = ''Config of the pulse daemon. See <literal>man pulse-daemon.conf</literal>.'';
};
config.environment.etc = [
{ target = "pulse/daemon.conf";
text = lib.generators.toKeyValue {} config.hardware.pulseaudio.daemon.config; }
];
}| { | ||
| target = "asound.conf"; | ||
| source = alsaConf; | ||
| } |
There was a problem hiding this comment.
{ target = "asound.conf";
source = alsaConf; }| ''; | ||
| }; | ||
|
|
||
| extraConfig = mkOption { |
There was a problem hiding this comment.
I suggest just having one config option instead of reproducing misc options of the daemon and reference to the manpage of the daemon for values & defaults. Or alternatively generate the config automatically from the sources. :P
|
I would be interested to hear opinions on @Profpatsch's proposal, it seems like a nice idea, but I think, we should stick with the common style across nixpkgs and roll out the change separately (maybe even on a larger scale). |
|
Implemented @Profpatsch's suggestions. |
This adds pulseaudio.daemon.config, which is a set of keys to values
which are directly translated to keys and values of pulseaudio's
daemon.conf, e. g.
hardware.pulseaudio.daemon.config = { flat-volumes = "no"; }
becomes
flat-volumes=no
in pulse/daemon.conf.
Motivation for this change
There is no possibilty to adjust the behavior of the pulseadio daemon via the daemon.conf file at the moment.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)This adds the following options:
pulseaudio.daemon.flat-volumes: wether to allow pulseaudio to adjustthe sink volume based on playback volumes
pulseaudio.daemon.resample-method: set pulseaudios resampling methodpulseaudio.daemon.extraConfig: extra config for daemon.confOpen question: Should we change the default value of flat-volumes?
The behavior has caused some controversy and Ubuntu as well as ArchLinux
chose to disable the feature.
Note: This is my first change involving
nixos/modulesand I am happy to receive suggestions to improve the code.