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

Conversation

peterhoeg
Copy link
Member

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

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
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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 {
Copy link
Member

@aanderse aanderse Jul 29, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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
Member

@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
6 tasks
@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

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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants