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

Add Jellyfin module #59465

Merged
merged 3 commits into from Apr 28, 2019
Merged

Add Jellyfin module #59465

merged 3 commits into from Apr 28, 2019

Conversation

minijackson
Copy link
Member

Motivation for this change

Emby is now proprietary (see #51832), Jellyfin is an active fork and already in nixpkgs unstable since PR #57046.

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.

As for the implementation, this is a simplified copy of the Emby module:

  • The pre-start script was removed in favor of specifying StateDirectory (at the cost of removing the dataDir option)
  • Also specify the cache directory
  • Add Jellyfin UID and GID

Notes:

I would have liked to use DynamicUser to avoid adding another UID and GID (we are dangerously approaching 399!), but since in a typical media server usecase, Jellyfin will have to access external directories, probably from another user, a static user and group is the simplest solution.

Another solution would have been to use DynamicUser, and add an option to specify extra static groups to add to the Jellyfin service (SupplementaryGroups), and let the user manage their media libraries accordingly.

One more minor detail is that I think the ffmpeg and ffprobe options now take two dashes instead of one and it isn't currently reflected in the jellyfin package. I don't mind changing that in this PR but it seemed somewhat out of place ^^

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.

One more minor detail is that I think the ffmpeg and ffprobe options now take two dashes instead of one and it isn't currently reflected in the jellyfin package. I don't mind changing that in this PR but it seemed somewhat out of place ^^

Either this PR or another seems reasonable to me 🤷‍♂️

nixos/modules/services/misc/jellyfin.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/jellyfin.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/jellyfin.nix Show resolved Hide resolved
nixos/modules/services/misc/jellyfin.nix Show resolved Hide resolved
nixos/modules/services/misc/jellyfin.nix Outdated Show resolved Hide resolved
@minijackson
Copy link
Member Author

Thanks for the review @aanderse ❤️ I'll check on the plugin directory, change the ffmpeg ffprobe options, and remove the "simple" type once I get home. If you think I'm wrong on the rest of the review I'd be happy too discuss it ^^

@aanderse
Copy link
Member

No problem. Looking good so far.

As a maintainer a simple NixOS test to ensure the service starts would make your life easier.

You should probably also add an assertion that the emby service isn't enabled as the two are mutually exclusive on port binding.

@aanderse aanderse mentioned this pull request Apr 21, 2019
10 tasks
@minijackson
Copy link
Member Author

Finally had some time to finish this! This should should solve all of your Jellyfin worries @aanderse, but let me know if I've missed things ^^

@aanderse
Copy link
Member

Awesome! Thanks for your work on this 😄

@aanderse aanderse merged commit 3fce35f into NixOS:master Apr 28, 2019
@minijackson minijackson deleted the module-jellyfin branch May 1, 2019 09:29
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