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.

Sign up for GitHub

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

Jump to bottom

xfce: delay package selection for pulseaudio volume to nixos modules #23382

Merged
merged 4 commits into from Dec 19, 2017
Merged

xfce: delay package selection for pulseaudio volume to nixos modules #23382

merged 4 commits into from Dec 19, 2017

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Mar 2, 2017

Motivation for this change

In XFCE, for some reasons, the current setup that selects whether to use xfce4volumed and xfce4volume-pulse and whether or not to enable pulseaudio support in xfce4mixer doesn't work at all. No matter what I set for hardware.pulseaudio.enable, no pulseaudio support is enabled. Selecting them in ./pkgs also makes it hard to install pulseaudio-enabled package hard when system-wise pulseaudio is not enabled (or on non-NixOS). Thus I suggest delaying the selection logic to the corresponding NixOS modules.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@bennofs
Copy link
Contributor

bennofs commented Apr 23, 2017 •
edited

Thanks, I agree that this ought to be controlled by the nixos config option, not the nixpkgs one. If anyone thinks this is an error, feel free to revert.

Edit: actually, i think we should keep both to maximize compatibility with existing configs.

bennofs
bennofs reviewed Apr 23, 2017
View reviewed changes
pkgs/desktops/xfce/default.nix
xfce4notifyd = callPackage ./applications/xfce4-notifyd.nix { };
xfce4taskmanager= callPackage ./applications/xfce4-taskmanager.nix { };
xfce4terminal = callPackage ./applications/terminal.nix { };
xfce4-screenshooter = callPackage ./applications/xfce4-screenshooter.nix { };
xfce4volumed = let
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep xfce4volumed for people that aren't using NixOS and are still using xfce applications from nixpkgs. Ditto for keeping the default for pulseaudioSupport for xfce4mixer above. With the changes in the nixos xfce module you made, this should have no effect on nixos but leave everything as it was before on non-nixos.

@lukateras
Copy link
Member

lukateras commented Oct 31, 2017 •
edited

Just add something to effect of config.nixpkgs.pulseaudio = lib.mkDefault config.hardware.pulseaudio.enable; to NixOS configuration, that'd ensure reverse compatibility. It's true that without proper care, PulseAudio is broken on Xfce desktops by default.

@lukateras
Copy link
Member

lukateras commented Oct 31, 2017 •
edited

PulseAudio is a per-user daemon, so it's a bad idea to only use config.hardware.pulseaudio.enable to decide this. Making it a default if config.hardware.libinput.enable is on is a good idea, though.

@lukateras
Copy link
Member

Actually, having a Nixpkgs option that selects volumed implementation is a bad idea at all. xfce4-volumed and xfce4-volumed-pulse are two separate programs, that don't share Git history or seemingly any of the code. Currently there is no way to install both.

@lukateras lukateras added the 6.topic: xfce The Xfce Desktop Environment label Dec 18, 2017
lukateras
lukateras approved these changes Dec 19, 2017
View reviewed changes
Copy link
Member

@lukateras lukateras left a comment •
edited

Choose a reason for hiding this comment

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

I think this is a very good idea.

@lukateras lukateras self-assigned this Dec 19, 2017
@lukateras
Copy link
Member

I've rebuilt system with this patch and both mixer and xfce4-volumed-pulse still work. Thank you a lot! It's much better than relying on (not properly documented) Nixpkgs option.

@lukateras lukateras merged commit 62a974b into NixOS:master Dec 19, 2017
@lukateras lukateras mentioned this pull request Dec 20, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennofs bennofs bennofs left review comments

@lukateras lukateras lukateras approved these changes

Assignees

@lukateras lukateras

Labels
6.topic: xfce The Xfce Desktop Environment 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sorpaas @bennofs @lukateras @GrahamcOfBorg