-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Add Jellyfin module #59465
Conversation
There was a problem hiding this 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 🤷♂️
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 ^^ |
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. |
11641a8
to
ab93789
Compare
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 ^^ |
Awesome! Thanks for your work on this 😄 |
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
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)As for the implementation, this is a simplified copy of the Emby module:
StateDirectory
(at the cost of removing thedataDir
option)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
andffprobe
options now take two dashes instead of one and it isn't currently reflected in thejellyfin
package. I don't mind changing that in this PR but it seemed somewhat out of place ^^