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

nixos: added navidrome service #91366

Closed
wants to merge 10 commits into from
Closed

nixos: added navidrome service #91366

wants to merge 10 commits into from

Conversation

aciceri
Copy link
Member

@aciceri aciceri commented Jun 23, 2020

Motivation for this change

I recently added the navidrome support with #88928, now should be nice to have the corresponding module service.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I was inspired by the airsonic and the transmission modules, I tested it and it works great.
However since this is my first NixOS module I'm sure that something needs to be improved, even for for this reasons the commits name aren't nice.
When someone will say that's fine I'll do git rebase to squash the commits into a single one with the correct message.

@rnhmjoj Since you have merged the navidrome package, maybe you are the most suitable for a review here. Thank you.

"d '${cfg.musicFolder}' '${cfg.musicFolderPermissions}' '${cfg.user}' '${cfg.group}' - -"
];

systemd.services.navidrome = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a good idea to reuse the upstream's systemd unit.

You should modify the package to install navidrome.service in $out/lib/systemd/system/, then to use the unit you can do

  systemd.packages = [ pkgs.navidrome ];

in the NixOS configuration. Now you only need to set the properties like environment and serviceConfig.User.

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.

A number of these options seem like they wouldn't often be used. There is a configuration file format (toml) as well. Maybe you could create a settings option to represent the configuration file which could replace a few of the options you have included in this module. See NixOS/rfcs#42 for rationale and some linked examples if you're interested.

If you're interested and need a hand ping me 👍

@aciceri
Copy link
Member Author

aciceri commented Jun 27, 2020

It would be a good idea to reuse the upstream's systemd unit

@rnhmjoj I understand your idea and approve it, however at the moment the package is built downloading the compiled binary, so I can't copy the systemd config file from the repository.
Obviously I could download both the binary and the repository, but since I was thinking to rewrite to package derivation to build the binary from the sources (as you suggested in another issue) I think to delay this idea for the future.
Is this ok for you?

Maybe you could create a settings option to represent the configuration file which could replace a few of the options you have included in this module.

@aanderse So do you suggest to fold all the settings inside a single settings option? I agree on this, however with the actual derivation every options has a default value which reflects the default value specified in the navidrome documentation. This is hardcoded, maybe an empty default value would be better, so navidrome could choose the default one and this module derivation would not depend on the package version.

Moreover, I was thinking to update the navidrome package too, can I simply add a commit to this pull request or I need to create a new one?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 30, 2020

Is this ok for you?

Yes, it's ok. In general it's better to lower the maintenance cost by using upstream-provided configurations but it's not a hard requirement.

This is hardcoded, maybe an empty default value would be better, so navidrome could choose the default one and this module derivation would not depend on the package version.

Yes, in fact this is one of the reasons for the push of settings option. The NixOS options provide good validation against common mistakes and simplify the configuration of the software. However, if there are many many settings, implementing all of the as NixOS options all and keeping them up to date can become a burden, so less common settings are better left to a settings option, which only provide validation against syntax and type errors.

@aciceri
Copy link
Member Author

aciceri commented Jul 1, 2020

@aanderse I'm trying your approach but I don't understand if the RFC you've cited is implemented. The examples in your link don't work.
However I subdivided the module options into:

  • user
  • group
  • musicFolderPermission (same approach of the transmission module)
  • virtualHost (default null)
  • settings which contains all the package settings documented on the official site

Then I overwrite settings with DataFolder = "/var/lib/navidrome/". At this point I need to build the TOML file starting from settings, the idea in the RFC is to do something like:

environment.etc."navidrome".source = formats.toml.format.generate pkgs "settings.toml" cfg.settings;

Then in systemd.services.navidrome.environment I could set ND_CONFIGFILE = environment.etc."navidrome/settings.toml" so when the service is started it correctly uses his config file.
The problem is that lib.formats doesn't even exists, I receive the error attribute 'formats' missing.
I saw that there exist toJSON but not toTOML: NixOS/nix#2967

I could put a custom writeTOML like here but I don't like to add useless dependencies.

writeTOML = name: x:
    pkgs.runCommandNoCCLocal name {
      passAsFile = ["config"];
      config = builtins.toJSON x;
      buildInputs = [ pkgs.go-toml ];
    } "jsontoml < $configPath > $out";

I'll push my new commit so you can read the code, obviously it doesn't work but it's to make you understand better if what I wrote is not enough.

@rnhmjoj Thank you for your answer, all clear. Obviously if you want to answer my last questions you're welcome too.

@aanderse
Copy link
Member

aanderse commented Jul 6, 2020

@aciceri I hate to make suggestions and not follow through, I thought it was going to be pretty straight forward in this case. I guess I was wrong. I'm a bit tapped out for time at the moment... I'm really hoping @infinisil will be kind enough to donate some of his time and guidance here. Pretty please with a 🍒 on top?

@deluan
Copy link

deluan commented Jul 6, 2020

I saw that there exist toJSON but not toTOML: NixOS/nix#2967

Hey @aciceri, Navidrome also supports JSON and YML config files, if it makes it easier for you. You just have to pass it as a parameter --configfile /path/navidrome.json or set it as an envvar ND_CONFIGFILE=/path/navidrome.json

jtojnar and others added 10 commits July 7, 2020 19:59
It is only optionally dlopened by crash reporter to try to get system proxy settings
but no system stores proxy in GConf these days so it is completely useless.

https://github.com/mozilla/gecko-dev/blob/97c590ed5532a7b382b5b8500c109de8f1d831e3/toolkit/crashreporter/client/crashreporter_gtk_common.cpp#L121-L122
It is only optionally dlopened by crash reporter to try to get system proxy settings
but no system stores proxy in GConf these days so it is completely useless.

https://github.com/mozilla/gecko-dev/blob/97c590ed5532a7b382b5b8500c109de8f1d831e3/toolkit/crashreporter/client/crashreporter_gtk_common.cpp#L121-L122
First working commit

Ops, was forgotting the module list

Proof of concept, not working, code provided only to ask for help

last
@aciceri aciceri closed this Jul 7, 2020
@aciceri aciceri deleted the navidrome-module branch July 7, 2020 18:10
@aciceri
Copy link
Member Author

aciceri commented Jul 7, 2020

Every time I try to git rebase I combine a disaster. I tried to delete the branch and to recreati it but I discovered that this automatically closes the pull request.
As soon as I create a new pull request I'll link it here.
Edit: #92621

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

5 participants