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

mpd: nixos module support for standard users #62771

Closed
wants to merge 4 commits into from
Closed

Conversation

@peterhoeg
Copy link
Member

peterhoeg commented Jun 6, 2019

Motivation for this change

Lots of new upstream activity.

Upstream is now using meson instead of autotools, so that's what we will use too.

There was a lot of redundant logic in order to deal with optional features (managing both flags and build inputs), so we now just add everything to buildinputs but use the flags to determine what is built in. If a dependency is not used, we don't have a reference to it either.

The nixos module now allows running mpd in a normal systemd user session (default is still system-wide) and we use the unit files from upstream instead of duplicating them.

Running fine here.

Cc: @astsmtl @Fuuzetsu @ehmry @fpletz

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tobim
Copy link
Contributor

tobim commented Jun 6, 2019

Please have a look at #57608.

@nixos-discourse
Copy link

nixos-discourse commented Jul 29, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/42

Copy link
Contributor

aanderse left a comment

I have only reviewed the module changes, but yeah this seems like the right direction to me. 👍

@@ -136,67 +144,73 @@ in {
parameter is omitted from the configuration.
'';
};

addBinariesToGlobalEnvironment = mkOption {

This comment has been minimized.

Copy link
@aanderse

aanderse Jul 29, 2019

Contributor

Personally I'm not a fan of options that simply add binaries to the global environment mostly because it is very simple to have the user do so themselves. I'm not sure if my opinion on this is popular or not. 🤷‍♂️

This comment has been minimized.

Copy link
@Mic92

Mic92 Jul 30, 2019

Contributor

I also don't think we need to have this option.

};
};

users.users = optionalAttrs (cfg.user == name) (singleton {
users.users = lib.mkIf cfg.startSystemWide (optionalAttrs (cfg.user == name) (singleton {

This comment has been minimized.

Copy link
@aanderse

aanderse Jul 29, 2019

Contributor

users.users = lib.mkIf cfg.startSystemWide (optionalAttrs (cfg.user == name) -> users.users = optionalAttrs (cfg.user == name && cfg.startSystemWide)

inherit description listenStreams wantedBy;
};

systemd.user.sockets.mpd = mkIf (!cfg.startSystemWide) {

This comment has been minimized.

Copy link
@aanderse

aanderse Jul 29, 2019

Contributor

It might help to comment on what's going on with these sockets as it isn't immediately obvious how this works in conjunction with your addition to systemd.packages.

@@ -54,6 +54,14 @@ in {
'';
};

startSystemWide = mkOption {

This comment has been minimized.

Copy link
@aanderse

aanderse Jul 29, 2019

Contributor

After some discussion involving tmpfiles and the appropriateness of specifying normal users for NixOS modules (which I'm almost certain you were a part of, sorry if I'm mistaken...) I think this option might become at least a little bit of a pattern going forward. That being said it is always good to have consistent naming conventions. startSystemWide is a very clear name so we should all agree to push this name as a loose 'standard' going forward, yeah?

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Jul 29, 2019

Author Member

the appropriateness of specifying normal users for NixOS modules

That's not really what this is. This is about making it available in a declarative way to normal users but not forcing it to run.

should all agree

I'm not sure that all those that should agree are seeing this... ;-)

@peterhoeg
Copy link
Member Author

peterhoeg commented Aug 13, 2019

Sorry about the noise guys, but I really need to find some time to clean this up. Everybody cool with leaving it open?

@aanderse
Copy link
Contributor

aanderse commented Aug 13, 2019

@peterhoeg of course that is fine! Let me know if you can't find the time and you need a hand 😄

@doronbehar doronbehar mentioned this pull request Nov 9, 2019
0 of 6 tasks complete
@fpletz fpletz changed the title mpd: 0.20.23 -> 0.21.10 + nixos module support for standard users mpd: nixos module support for standard users Nov 13, 2019
@fpletz
Copy link
Member

fpletz commented Nov 13, 2019

Since #57608 was merged, this PR is only for user session support. Cherry-picked your mpc bump to master: 1eea0f5

@peterhoeg
Copy link
Member Author

peterhoeg commented May 6, 2020

Closing this for 2 reasons:

  1. the packages have already been updated past this, and
  2. I believe things like mpd really belongs in home-manager, which already has a module
@peterhoeg peterhoeg closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.