jellyfin: update to 10.4.1, port test to python

[minijackson]

Motivation for this change

Release notes:

https://github.com/jellyfin/jellyfin/releases/tag/v10.4.1

Please note that there is a potential compatibility issue with the base URL (explained in detail, with fix in the release notes).

Link to python "meta" PR: #72828

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 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)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

Notify maintainers

cc @nyanloutre

[worldofpeace]

The test looks good, though the issues with base url look pretty difficult.
Every person on unstable and 20.03 in the future would have to do the steps outlined in the Release Note?
I'm not sure how we could handle this migration in nixos. Ideally the module will perform this when jellyfin matches the version constraint it which it needs to be performed.

[worldofpeace]

@minijackson Perhaps in preStart of the service the nixos module does

Find and open system.xml in the Jellyfin configuration directory (e.g. /etc/jellyfin/system.xml)
Find the entry <BaseUrl>.
Replace the entire line, i.e. <BaseUrl>/jellyfin</BaseUrl>, with <BaseUrl/>.

and condition it on the jellyfin version.

[flokli]

I'm not feeling very well with adding that migration code, and keeping it around forever. It might also have undesired side effects.

I'd prefer adding a nixos release note mentioning the jellyfin upgrade, and asking people to update their config, with a link to the upstream changelog.

[alyssais]

Release notes don’t help unstable users much. Even if we put the upgrade in for a few months, that would give unstable users, who probably don’t read the release notes, a chance to have their configuration fixed. Stable users will likely read the release notes on upgrade and can make this change themselves.

[minijackson]

On the more technical side of things, how would we proceed on implementing this? I think the best would be to remove the BaseUrl only if we upgraded from 10.4.0, and only if the BaseUrl is /jellyfin, but even then, this might be what the end user wanted.

I don't know how you would check that we just did the upgrade path 10.4.0 -> 10.4.1

I'm not really fond of adding code that modifies user settings without their knowledge

I just checked using the nixos test, the default BaseUrl for 10.4.1 is empty, so it really seems that any user that does any other upgrade path than 10.4.0 -> 10.4.1 shouldn't be affected.

I'm not familiar with the rules of the unstable channel, but I think it should be reasonnable to leave it as is, and let affected users look for the release notes or this PR, instead of introducing changes that would potentially affect everyone. I'm open for discussion though ^^

[minijackson]

hum, I did some more speluking, and I found that the version in NixOS master (10.4.0) is borked?

using this config:

{ ... }:
{
  services.jellyfin.enable = true;
  networking.firewall.enable = false;
}

And from the VM terminal:

[root@machine:~]# curl -v localhost:8096
*   Trying ::1:8096...
* TCP_NODELAY set
* Connected to localhost (::1) port 8096 (#0)
> GET / HTTP/1.1
> Host: localhost:8096
> User-Agent: curl/7.66.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 302 Found
< Date: Sat, 09 Nov 2019 09:24:05 GMT
< Server: Kestrel
< Content-Length: 0
< Location: //web/index.html
<
* Connection #0 to host localhost left intact

This redirects to //web/index.html, which is not good :-/

[worldofpeace]

One of the things I liked about nixos was manual migration could usually be handled in nixos. but since this involves user settings, I don't think there's much we can do aside from users having to figure this out. Though as @alyssais mentioned, having a release note isn't going to help people using unstable.

I'm not sure what to do for unstable users... The only thing better I can think of is opening an issue with these details. We lack a way to give News items to unstable users, so we could at least make it simple for them from searching on github.

I can think of a bunch of more ugly options, but I don't think I would want to request someone to actually pursue them. As this stands, we lack a way to support unstable users in this way, so likely they're familiar with traversing github 😄

Issue title could be something like

jellyfin>=10.4.1 update requires manual intervention

[flokli]

I think "unstable" implies things might break, and we aim to add release notes in the same PRs that change behaviour. People using the unstable channel should be able to traverse GitHub, at least open an issue, better, use the search functionality ;-) I have some ideas in terms of improving the docs, including the unstable docs, so release notes for unstable could be shown there, too.

[alyssais]

I had (wrongly?) assumed that migrating here would be a simple XML modification we could detect the need for in a preStart and do idempotently. If that's not the case here then I agree just not attempting to do any migration is the best.

[minijackson]

For me, the summary is that:

• The bug will affect only users that are currently in 10.4.0
• To my knowledge 10.4.0 is not currently in any stable branch, so no mention in release notes is necessary (but I don't mind a mention to the bug anyway, just in case ^^)
• It would be nice to automatically prevent it (obviously) but:
  • The value to check for is a perfectly valid configuration, and may be wanted by the end user
  • It should be changed only on a very specific upgrade

Considering that, if we're deciding on not doing the migration script, I think the best course of action would be to merge the upgrade as fast as possible, to prevent more people from going to 10.4.0, and hence encountering the bug.

[flokli]

I agree. Let's merge this in.

[flokli]

Thanks!