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

pulseaudio: Add support for package configuration files. #16834

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Jookia
Copy link
Contributor

Jookia commented Jul 10, 2016

In a fashion like udev's support, this patch allows configurations from packages
to be merged in to directories for PulseAudio to read from. Currently supported
directories are the alsa-mixer mdoule's profile-sets and paths.

This is accomplished by patching PulseAudio to read directories from environment
variables globally defined by NixOS rather from the data path in the Nix store.
Modules that support it (such as the alsa module) can still be configured at
runtime to use specific paths, this just changes the default paths.

The environment variables are only used if they're defined, as such the previous
behaviour can be reverted to if the variables are unset or NixOS isn't running.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Jul 10, 2016

@Jookia, thanks for your PR! By analyzing the annotation information on this pull request, we identified @wkennington, @rickynils and @edolstra to be potential reviewers

@abbradar

View changes

nixos/modules/config/pulseaudio.nix Outdated

for i in ${toString cfg.packages}; do
echo "Adding configuration for package $i"
copy_dir $i/usr/share/pulseaudio/alsa-mixer profile-sets alsa-profiles

This comment has been minimized.

@abbradar

abbradar Jul 11, 2016

Member

Using /usr is undesirable in packages -- can this be fixed to just /share/pulseaudio?

This comment has been minimized.

@Jookia

Jookia Jul 11, 2016

Author Contributor

Sure! I only did this to make my packaging easier

@abbradar

This comment has been minimized.

Copy link
Member

abbradar commented Jul 11, 2016

I'm curious, when are those files needed? Any example of a package which can be added to environment?

@abbradar

View changes

nixos/modules/config/pulseaudio.nix Outdated
@@ -135,6 +177,7 @@ in {

(mkIf cfg.enable {
environment.systemPackages = [ cfg.package ];
environment.sessionVariables = moduleEnvVars;

This comment has been minimized.

@abbradar

abbradar Jul 11, 2016

Member

An idea for potential improvement: instead add to environment not raw cfg.package but a wrapped one with makeWrappered binaries adding those variables. Less pollution in the environment and more robustness.

This comment has been minimized.

@Jookia

Jookia Jul 11, 2016

Author Contributor

I'm not sure what you mean here exactly- the module packages have no binaries, and these variables just tell pulseaudio where to look for the module configuration. If you mean wrapping pulseaudio itself to look for moduleEnvVars wouldn't that require building pulseaudio each change?

This comment has been minimized.

@abbradar

abbradar Jul 11, 2016

Member

I mean wrapping pulseaudio, but not itself -- rather as a part of a wrapper (see deadbeef, pidgin, dwarf-therapist etc.) The main idea is:

symlinkJoin {
  name = "pulseaudio-with-packages";
  paths = [ cfg.package ] ++ cfg.packages;
  buildInputs = [ makeWrapper ];
  postBuild = ''
    wrapProgram $out/bin/pulseaudio \
      --set PA_ALSA_PATHS_DIR ... \
      --set PA_ALSA_PROFILE_SETS_DIR ....
  '';
}
, and this goes directly to `systemPackages`. This drops need for system-wide environment variables and may make the whole thing simpler (because most of `buildCommand` code above should not be needed this way -- if I'm not mistaking anything!).

This comment has been minimized.

@Jookia

Jookia Jul 12, 2016

Author Contributor

Sounds like a good idea :) I'll implement it along with your other suggestions when I can then redo my commit.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Jul 11, 2016

An example package is https://github.com/xobs/pulseaudio-novena which is the main reason I've done this

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 1, 2016

So months later I've come back to this finally since the current patch is broken beyond belief outside my ARM machine.

Wrapping the pulseaudio binaries causes some grief, but ultimately isn't doable because some packages use 'cfg.pulseaudio.package' to run pulseaudio binaries, which wouldn't be the wrapped version. Instead I've dropped the environment variables patch and now just get pulseaudio to look in /share/pulse/alsa-mixer for the alsa-mixer data which is generated the same, this is done by editing the configure.ac file to change the defines.

The only other way to do this would be to get pulseaudio's alsa-mixer module to support taking an paths-dir argument. Indeed, some of the code looks to be set up for such a situation. Doing this would mean it'd go in the default.pa file generated. This would be the ideal situation, but it doesn't solve the issue for other modules.

I'll put up a patch in a week or so when I can test it out on my ARM machine where it actually needs to work.

pulseaudio: Add support for package configuration files.
In a fashion like udev's support, this patch allows configurations from packages
to be merged in to directories for PulseAudio to read from. Currently supported
directories are the alsa-mixer mdoule's profile-sets and paths.

This is accomplished by configuring the PulseAudio daemon to read directories
from /etc/pulse/alsa-mixer, and having NixOS generate that directory.
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 28, 2016

27 days later I've pushed the newer cleaned up version, which works on my ARM machine. It's much simpler and does the job.

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Dec 2, 2018

@Jookia It looks like this has gone under the radar for way too long… are you still interested in moving this forward?

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Dec 2, 2018

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Dec 2, 2018

OK thank you! sorry for not having moved that forward when you were still willing to push it forward, and hope someone else will come upon this if they want the same thing :)

@Ekleog Ekleog closed this Dec 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.