jellyfin 10.5.5 -> 10.6.0 #93654

Church- · 2020-07-22T15:47:11Z

Motivation for this change

Jellyfin 10.6.0 was released, large feature set change

Things done

lsix · 2020-07-23T07:41:24Z

Hi, Thanks for the contribution!

Apparently, the homepage redirects to https://jellyfin.org/. Maybe you can update the meta.homepage attribute too.

Appart from that, this looks good to me.

lsix · 2020-07-23T07:47:57Z

I have also checked the nixos test (nix-build nixos/tests/jellyfin.nix).

Church- · 2020-07-23T13:29:12Z

Sure, let me update that!

minijackson · 2020-07-23T13:44:00Z

The release notes strongly suggest doing backups of the state directory in case of a rollback, which can't be done automatically from this new version. Is there something we should do to prevent some unwanted surprises for users of the jellyfin module?

In my opinion, it's also something to keep in mind for the NixOS release notes, too

Church- · 2020-07-23T17:32:07Z

I was going to suggest adding a blurb in the release notes that you should make a backup, etc, etc.
I'm wondering if the service can't be patched to make a backup on next run.
cp -r /var/lib/jellyfin /var/lib/jellyfin.backup or something. Stick it with a conditional in a preExec step in the service.

Church- · 2020-07-24T16:59:10Z

Something like the above could possibly suffice for at least allowing the users to rollback if needed after upgrading to 10.6.0.

Along with a by-line in the release notes for this package.

minijackson · 2020-07-24T19:04:11Z

I'm personally not a fan of doing the backups this way, it seems to me a bit orthogonal to the nixos way: not configurable, and done without mentioning it to the users. The only way for them to see that this backup is done is either by being aware of this PR, reading the source code before nixos-rebuild switch, or reading the release notes when they are here.

It also introduces a new directory under /var/lib, which kinda breaks some assumptions about jellyfin data when you read the systemd unit file?

But, I might need a second opinion on this

Church- · 2020-07-24T20:22:31Z

That is true, and I do agree.
Maybe include it as an option?
Something like:

jellyfin = {
  enable_backups = true;
   }

If set to true then tar.gz up the dir and place it somewhere.
Where I'm not certain.

Could model it off a similar option in gitea.

But really it seems like folks are going to either need to read the release notes at some point or be aware of breaking changes. There's only so much we can do I think.

Unless we can have some way to detect they're upgrading to 10.6.0 and backup their data dir + dump a message to stdout for them.

minijackson · 2020-07-28T12:05:22Z

Jellyfin 10.6.1 released a few hours ago, could you update the derivation?

lsix · 2020-07-28T12:14:17Z

Reguarding the question of the module storage part, one approach that can be to do something like:

      serviceConfig = rec {
        User = cfg.user;
        Group = cfg.group;
        StateDirectory = "jellyfin";
        CacheDirectory = "jellyfin";
        db_version = pkgs.lib.versions.majorMinor pkgs.jellyfin.version;
        ExecStart = "${pkgs.jellyfin}/bin/jellyfin --datadir '/var/lib/${StateDirectory}/${db_version}' --cachedir '/var/cache/${CacheDirectory}'";
        Restart = "on-failure";
      };

This is the kind of approach that is used for postgresql. This is not perfect since after an upgrade you tend to have old state dir that dangle around forever. But if the main concern is about keeping old version of the data before a major upgrade, it kind of does the trick (considering that you never actually upgrade, but only create a fresh instance).

Church- · 2020-07-28T16:45:11Z

Reguarding the question of the module storage part, one approach that can be to do something like:
      serviceConfig = rec {
        User = cfg.user;
        Group = cfg.group;
        StateDirectory = "jellyfin";
        CacheDirectory = "jellyfin";
        db_version = pkgs.lib.versions.majorMinor pkgs.jellyfin.version;
        ExecStart = "${pkgs.jellyfin}/bin/jellyfin --datadir '/var/lib/${StateDirectory}/${db_version}' --cachedir '/var/cache/${CacheDirectory}'";
        Restart = "on-failure";
      };
This is the kind of approach that is used for postgresql. This is not perfect since after an upgrade you tend to have old state dir that dangle around forever. But if the main concern is about keeping old version of the data before a major upgrade, it kind of does the trick (considering that you never actually upgrade, but only create a fresh instance).

That actually is a good idea. Wonderful, thank you. I'll make those modifications soon as I get the chance today.

@lsix My only question is, does this mean setup will have to be done anew after every upgrade?

Jellyfin 10.6.1 released a few hours ago, could you update the derivation?

Yep definitely, should help some with the mem leaks this version can have.

lsix · 2020-07-29T12:35:36Z

It overall looks good. From what I see:

You should squash your commits. Probably into 2 commits: one for the package update, and one for the module change,
The state dir could probably be a module option. This way someone can just choose to set a version-independent folder and never worry about it (but I guess this is an option),
You should probably add a release note in nixos/doc/manual/release-notes/rl-2009.xml to warn users of the configuration change

(If you need help with point 2, reach out)

@lsix My only question is, does this mean setup will have to be done anew after every upgrade?

As it is, yes (for every « major » upgrade ; not for a minor, such as 10.6.0 -> 10.6.1). We might want to be a bit more clever about this, and maybe ass a preStart script that will check if the current state dir is empty and if an older one can be found, then copy from the old one to the new and let jellyfin take care of the internal structure upgrade.

Church- · 2020-07-29T14:54:16Z

It overall looks good. From what I see:
* You should squash your commits. Probably into 2 commits: one for the package update, and one for the module change,

* The state dir could probably be a module option. This way someone can just choose to set a version-independent folder and never worry about it (but I guess this is an option),

* You should probably add a release note in `nixos/doc/manual/release-notes/rl-2009.xml` to warn users of the configuration change
(If you need help with point 2, reach out)

@lsix My only question is, does this mean setup will have to be done anew after every upgrade?

As it is, yes (for every « major » upgrade ; not for a minor, such as 10.6.0 -> 10.6.1). We might want to be a bit more clever about this, and maybe ass a preStart script that will check if the current state dir is empty and if an older one can be found, then copy from the old one to the new and let jellyfin take care of the internal structure upgrade.

All good points, a preStart step might be a good idea ya.

As far as squashing git commits, things are somewhat inter leavened I believe. Package updates and module updates in the same commits.

Guess the best way might be reverting/deleting the module updates in a new commit, squash, and then make a new commit with module updates ya?

Church- · 2020-08-02T03:48:06Z

@lsix Already, redid the commits as per your suggestion. One for the package update.One for the module.

For the module there's a... fairly gnarly shellscript in the PreStart stage of the service.
It'll:

ensure there's a dir of /var/lib/jellyfin/database/db_version
if the version is 10.6.1 (commit where the breaking change occurs) just copy over the database into the correct database/db_version dir.
If it's not equal to that version (presumed greater since we didn't track it till this update)
- Get all the db versions, and reverse sort them to get the latest version (Maybe use the version sort flag here?)
- loop through and check if the version dir is actually a dir.
- check if the version dir has contents
- copies them to the new db_version dir if it does have contents.

Thoughts?

lsix

Few remarks.

As mentioned by @minijackson this approach is not completely usual in the nixos ecosystem. Having this behavior governed by an option (jellyfin.versioned_statedir or something else, I am terrible with finding good concise names) could be beneficial. In many systems, it would make sense to just use the service as it is currently, and rely on a generic backup system aside (I guess).

I do not really have a strong opinion on where this should go, so I’ll let @minijackson decide as module maintainer ^_^

Anyway @Church- thanks a lot for your contribution and work, sorry this takes so long.

lsix · 2020-08-03T07:39:00Z

nixos/modules/services/misc/jellyfin.nix

+            cp -r /var/lib/${StateDirectory}/data /var/lib/${StateDirectory}/database/${db_version}/
+            exit 0
+          else
+            versions=$(find /var/lib/${StateDirectory}/database/ -maxdepth 1 -mindepth 1 | sort -r)


Sort might not work as expected here:

echo -e "10.8\n10.9\n10.10"|sort -r 10.9 10.8 10.10

lsix · 2020-08-03T08:04:02Z

nixos/modules/services/misc/jellyfin.nix

+            do
+              if [ -d "$version" ]; then
+                if find "$version" -mindepth 1 -print -quit 2>/dev/null | grep -q .; then;
+                  cp -r "$version" /var/lib/${StateDirectory}/database/${db_version}


If we have this sort of thing, I guess we might want a clean-up mechanism (or not, I really do not know). Maybe drop all but 2 most recent?

lsix · 2020-08-03T08:13:02Z

nixos/modules/services/misc/jellyfin.nix

+        ExecPreStart = ''
+          #!/bin/bash
+          mkdir -p /var/lib/${StateDirectory}/database/${db_version}
+          if [ "${db_version}" = "10.6.1" ]; then


maybe

if [ -d "/var/lib/${StateDirectory}/data" ]

would be more direct and also more generic (not version specific).

lsix · 2020-08-03T08:17:45Z

nixos/modules/services/misc/jellyfin.nix

+          #!/bin/bash
+          mkdir -p /var/lib/${StateDirectory}/database/${db_version}
+          if [ "${db_version}" = "10.6.1" ]; then
+            cp -r /var/lib/${StateDirectory}/data /var/lib/${StateDirectory}/database/${db_version}/


I have not checked, but what happens to everything that does not fall into data? (log and so on)

minijackson · 2020-08-03T12:34:30Z

Ultimately, I think it should be up for the user / sysadmin to decide how and if they want to backup the jellyfin state directory. One solution that could be nice is to do it the PostgreSQL way, and use the system.stateVersion option to check if we want to upgrade to 10.6+, by adding the package option, and doing something like this:

{
  services.jellyfin.package = mkDefault (
    if versionAtLeast config.system.stateVersion "20.09" then pkgs.jellyfin
    else pkgs.jellyfin_10_5);
}

That would mean we would also have to add the jellyfin_10_5 pkg, and print a friendly warning to the user if the stateVersion is less than 20.09.

Sorry to ask you that again, but could you also update the version to the recently released 10.6.2?

Church- · 2020-08-03T14:23:02Z

Ultimately, I think it should be up for the user / sysadmin to decide how and if they want to backup the jellyfin state directory. One solution that could be nice is to do it the PostgreSQL way, and use the system.stateVersion option to check if we want to upgrade to 10.6+, by adding the package option, and doing something like this:
{
  services.jellyfin.package = mkDefault (
    if versionAtLeast config.system.stateVersion "20.09" then pkgs.jellyfin
    else pkgs.jellyfin_10_5);
}
That would mean we would also have to add the jellyfin_10_5 pkg, and print a friendly warning to the user if the stateVersion is less than 20.09.

Sorry to ask you that again, but could you also update the version to the recently released 10.6.2?

Given we want to waylay any possible db corruptions/rollback issues.

Maybe defaulting to pkgs.jellyfin10_5 and having to opt in to pkgs.jellyfin without the dep rectifying above.
Thoughts @minijackson? Otherwise I'm all for your approach.

minijackson · 2020-08-03T16:03:10Z

Do you mean defaulting the package option to the old version, and letting the user override that?

I'm personally more fond of the "default to the old one only if stateVersion < 20.03" solution: it makes new installations much simpler/cleaner, and there is very little chance of someone wanting to rollback or having db corruption issues if the user didn't have a jellyfin service <= 10.5.x

But if you feel I'm missing something, feel free to tell!

Church- · 2020-08-03T17:22:16Z

Hmm, okay. No I think you do have the right of it ya.

I'll go with that implementation idea.

DIzFer · 2020-08-13T14:44:42Z

Upgrade-wise this WFM, no related issues in the last weeks, beyond upgrading to 10.6.2 because of a 404.

I also agree using stateVersion is the way to go.

Church- · 2020-08-15T04:35:29Z

@lsix Finally pushed those changes, if you're happy with them then I think we're good to merge down.

minijackson · 2020-08-16T09:35:58Z

Thank you! Small nitpick: according to the CONTRIBUTING, I think your last commit should start with nixos/jellyfin:, otherwise, I'm good with the changes.

minijackson · 2020-08-16T09:41:33Z

Just did a nix-review, and got an error:

hash mismatch in fixed-output derivation '/nix/store/arpdb9sxb95xl79bh9yljmmw3alaas0g-jellyfin_10.6.2.tar.gz':
  wanted: sha256:0dw9fvxwpbm4pn5m8lqvn7nzysn8ks9n92frs3pidifvxlnzawzz
  got:    sha256:16yib2k9adch784p6p0whgfb6lrjzwiigg1n14cp88dx64hyhxhb

minijackson · 2020-08-16T11:12:50Z

nixos/modules/services/misc/jellyfin.nix

+      package = mkDefault (
+        if versionAtLeast config.system.stateVersion "20.09" then pkgs.jellyfin
+        else pkgs.jellyfin_10_5
+      );


Suggested change

package = mkDefault (

if versionAtLeast config.system.stateVersion "20.09" then pkgs.jellyfin

else pkgs.jellyfin_10_5

);

package = mkOption {

type = types.package;

example = literalExample "pkgs.jellyfin";

description = ''

Jellyfin package to use.

'';

};

then you can add this to the configuration part:

services.jellyfin.package = mkDefault (if versionAtLeast config.system.stateVersion "20.09" then pkgs.jellyfin else pkgs.jellyfin_10_5);

and finally, you can replace pkgs.jellyfin by cfg.package in the ExecStart line of the configuration part.

Church- · 2020-08-16T13:12:49Z

Ugh, dumb mistakes, I'll push those up in a few.

Church- · 2020-08-16T13:56:41Z

@minijackson And those are fixed. Dumb mistakes, thanks for catching them.

minijackson

Pro-tip: you can do nix build -L -f ./nixos/tests/jellyfin.nix to execute the jellyfin unit-test yourself

minijackson · 2020-08-16T14:09:48Z

nixos/modules/services/misc/jellyfin.nix

+      package = mkOption (
+        type = types.package;
+        example = literalExample "pkgs.jellyfin";
+        description = ''
+          Jellyfin package to use.
+        '';
+      );


Suggested change

package = mkOption (

type = types.package;

example = literalExample "pkgs.jellyfin";

description = ''

Jellyfin package to use.

'';

);

package = mkOption {

type = types.package;

example = literalExample "pkgs.jellyfin";

description = ''

Jellyfin package to use.

'';

};

minijackson · 2020-08-16T14:09:58Z

nixos/modules/services/misc/jellyfin.nix

        Restart = "on-failure";
      };
    };

+    package = mkDefault (


Suggested change

package = mkDefault (

services.jellyfin.package = mkDefault (

Church- · 2020-08-16T14:51:45Z

@minijackson And fixed yet again, you know with all the module writing I've done the past few weeks you'd think I wouldn't make such stupid mistakes. 😛

minijackson · 2020-08-16T15:10:13Z

No worries ^^

Everything works now when system.stateVersion = "20.09"

We still have issues for when stateVersion is less than 20.09:

we need to add the jellyfin_10_5 package to all-packages.nix
the jellyfin_10_5 package needs to be version 10.5.x, currently it is 10.6.1 ^^

Also, could you put the jellyfin_10_5 package into pkgs/servers/jellyfin/10.5.x.nix instead of pkgs/servers/jellyfin_10_5/default.nix? I think this would be more in line with the rest of nixpkgs.

Sorry that the review is in such a back and forth manner...

Church- · 2020-08-16T15:11:56Z

@minijackson Nah that's perfectly fine, first time PR'ing to nix, so there are things to learn. 😄
Thanks for taking the time to teach me.

Changes incoming.

Church- · 2020-08-16T15:15:48Z

@minijackson For all-packages.nix would I invoke jellyfin_10_5 via: jellyfin = callPackage ../servers/jellyfin/10.5.x.nix { }; ?

minijackson · 2020-08-16T15:17:46Z

Almost! If I'm not wrong, it would be jellyfin_10_5 = callPackage ../servers/jellyfin/10.5.x.nix { }

Church- · 2020-08-16T15:21:11Z

And pushed again!

pkgs/top-level/all-packages.nix

Added jellyfin_10_5 package as well to be used for the module in nixos versions < 20.09

…ts to using the default jellyfin package if nixos version is 20.09 or greater, otherwise will default to using the new jellyfin_10_5 derivation for older systems.

Church- · 2020-08-16T15:43:49Z

And hopefully the last typo is fixed.

minijackson · 2020-08-16T15:54:18Z

Perfect! Both packages build, and the test pass with either stateVersion = "20.09" or stateVersion = "20.03"!

Do you still want to be writing something for the 20.09 release notes?

If you do, I think you need to add to the file nixos/doc/manual/release-notes/rl-2009.xml. If you don't, no worries, I don't mind writing to it either ^^

Church- · 2020-08-16T16:27:43Z

Edit: Actually I'll let you do the Release Notes if you don't mind? :)
Would be a good chance to see what the structure of it should look like for future releases.

I'll add to the release notes! Change incoming.
I'll include release notes as a separate commit?

minijackson · 2020-08-16T17:23:29Z

Actually I'll let you do the Release Notes if you don't mind? :)

No problem! I think we're good to merge, then.

Church- · 2020-08-16T19:03:15Z

So what's the merge process like? Get someone with write access to do it?

minijackson · 2020-08-16T19:41:54Z

So what's the merge process like? Get someone with write access to do it?

Yes, it's usually as simple as waiting a few days, but if it takes too long, there's a thread on NixOS's discourse for already reviewed PR: https://discourse.nixos.org/t/prs-already-reviewed/2617

Church- · 2020-08-16T19:45:11Z

Got it, cool. :)

nixos-discourse · 2020-08-18T16:29:35Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

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

ofborg bot requested a review from nyanloutre July 22, 2020 15:55

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 22, 2020

ofborg bot added 6.topic: nixos 8.has: module (update) labels Jul 24, 2020

lsix reviewed Aug 3, 2020

View reviewed changes

minijackson reviewed Aug 16, 2020

View reviewed changes

minijackson suggested changes Aug 16, 2020

View reviewed changes

minijackson reviewed Aug 16, 2020

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

backtrace-ci-bot added 2 commits August 16, 2020 11:41

jellyfin: 10.5.5 -> 10.6.2

6a71fd3

Added jellyfin_10_5 package as well to be used for the module in nixos versions < 20.09

nixos/jellyfin: added a package option to the options section, defaul…

ce9f0c4

…ts to using the default jellyfin package if nixos version is 20.09 or greater, otherwise will default to using the new jellyfin_10_5 derivation for older systems.

ofborg bot added 8.has: package (new) and removed 10.rebuild-linux: 1 labels Aug 16, 2020

minijackson approved these changes Aug 16, 2020

View reviewed changes

AndersonTorres merged commit e7139f4 into NixOS:master Aug 19, 2020

minijackson mentioned this pull request Aug 22, 2020

nixos/jellyfin: document stateVersion 20.09 in release notes #95987

Merged

10 tasks

minijackson mentioned this pull request Apr 24, 2021

jellyfin_10_5: remove unmaintained version #120520

Merged

10 tasks

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

	package = mkDefault (
	services.jellyfin.package = mkDefault (

jellyfin 10.5.5 -> 10.6.0 #93654

jellyfin 10.5.5 -> 10.6.0 #93654

Conversation

Church- commented Jul 22, 2020 • edited by lsix

Motivation for this change

Things done

lsix commented Jul 23, 2020

lsix commented Jul 23, 2020 • edited

Church- commented Jul 23, 2020

minijackson commented Jul 23, 2020

Church- commented Jul 23, 2020 • edited

Church- commented Jul 24, 2020 • edited

minijackson commented Jul 24, 2020 • edited

Church- commented Jul 24, 2020 • edited

minijackson commented Jul 28, 2020

lsix commented Jul 28, 2020

Church- commented Jul 28, 2020 • edited

lsix commented Jul 29, 2020

Church- commented Jul 29, 2020

Church- commented Aug 2, 2020

lsix left a comment

Choose a reason for hiding this comment

lsix Aug 3, 2020

Choose a reason for hiding this comment

lsix Aug 3, 2020

Choose a reason for hiding this comment

lsix Aug 3, 2020

Choose a reason for hiding this comment

lsix Aug 3, 2020

Choose a reason for hiding this comment

minijackson commented Aug 3, 2020

Church- commented Aug 3, 2020

minijackson commented Aug 3, 2020 • edited

Church- commented Aug 3, 2020

DIzFer commented Aug 13, 2020 • edited

Church- commented Aug 15, 2020

minijackson commented Aug 16, 2020

minijackson commented Aug 16, 2020

minijackson Aug 16, 2020

Choose a reason for hiding this comment

Church- commented Aug 16, 2020

Church- commented Aug 16, 2020

minijackson left a comment

Choose a reason for hiding this comment

minijackson Aug 16, 2020

Choose a reason for hiding this comment

minijackson Aug 16, 2020

Choose a reason for hiding this comment

Church- commented Aug 16, 2020

minijackson commented Aug 16, 2020

Church- commented Aug 16, 2020

Church- commented Aug 16, 2020

minijackson commented Aug 16, 2020

Church- commented Aug 16, 2020

Church- commented Aug 16, 2020

minijackson commented Aug 16, 2020

Church- commented Aug 16, 2020 • edited

minijackson commented Aug 16, 2020

Church- commented Aug 16, 2020

minijackson commented Aug 16, 2020

Church- commented Aug 16, 2020

nixos-discourse commented Aug 18, 2020

Church- commented Jul 22, 2020 •

edited by lsix

lsix commented Jul 23, 2020 •

edited

Church- commented Jul 23, 2020 •

edited

Church- commented Jul 24, 2020 •

edited

minijackson commented Jul 24, 2020 •

edited

Church- commented Jul 24, 2020 •

edited

Church- commented Jul 28, 2020 •

edited

minijackson commented Aug 3, 2020 •

edited

DIzFer commented Aug 13, 2020 •

edited

Church- commented Aug 16, 2020 •

edited