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

sonarr: 3.0.10.1567 -> 4.0.0.748 #278050

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

purcell
Copy link
Member

@purcell purcell commented Jan 1, 2024

Description of changes

As pointed out in #278002 we were lagging a long way behind the latest releases, with v4 coming out in Nov 2022: https://forums.sonarr.tv/t/sonarr-v4-beta/31078

There's no longer a plain "linux" (actually cross-platform) package available for download, so I made an attempt to build from source (like jellyfin) but abandoned it due to obscure issues with an apparently-vendored .NET library, instead opting to follow the pattern of per-platform downloads used for radarr.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@purcell
Copy link
Member Author

purcell commented Jan 1, 2024

Perhaps you'd like to give this a try, @dacioromero.

@purcell
Copy link
Member Author

purcell commented Jan 1, 2024

Open questions from my side:

  • The 4.x release claims to not need mono, so I tried switching the wrapper script to dotnet-runtime like radarr, but didn't get it working, and in the meantime the program appears to launch find as-is
  • Unsure if the on-disk config is compatible, and whether it's therefore potentially necessary to tag the config version

@purcell purcell force-pushed the sonarr-4 branch 2 times, most recently from 16e063e to 615d5ce Compare January 1, 2024 16:19
pkgs/servers/sonarr/default.nix Show resolved Hide resolved
pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
@justinas
Copy link
Member

justinas commented Jan 1, 2024

Stable release announcement.

Some points of note:

ffprobe has replaced MediaInfo - no more crashes from failed media scans

We probably need to add ffmpeg to sonarr's PATH?

Due to multiple database migrations we’ve seen that some corrupt databases that were doing OK in v3 have broken in v4.

We probably need to at least note the possible breakage in the 24.05 release notes.

@purcell
Copy link
Member Author

purcell commented Jan 1, 2024

I've force-pushed a couple of changes trying to get this to run on my aarch64-linux machine, but no success so far. Launching with mono reported that the executable was not a valid assembly, but launching the Sonarr executable directly doesn't work either because it's dynamically linked:

Jan 01 16:23:24 media NzbDrone[397086]: Could not start dynamically linked executable: /nix/store/kppfskw8fwm6gsdhj594lla9vnnmnaii-sonarr-4.0.0.748/bin/Sonarr
Jan 01 16:23:24 media NzbDrone[397086]: NixOS cannot run dynamically linked executables intended for generic
Jan 01 16:23:24 media NzbDrone[397086]: linux environments out of the box. For more information, see:
Jan 01 16:23:24 media NzbDrone[397086]: https://nix.dev/permalink/stub-ld

In an ideal world we'd just build from source, but that started to feel painful. I pushed my changes to a branch in case anyone is interested.

@purcell purcell force-pushed the sonarr-4 branch 6 times, most recently from b14706f to 1a12f0a Compare January 1, 2024 18:03
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Jan 1, 2024
@purcell
Copy link
Member Author

purcell commented Jan 1, 2024

I think I have this working now, using the dotnet-runtime to run the DLL, and adding the necessary runtime dependencies (ffmpeg, as suggested, plus icu and openssl). And I applied the inherit hash suggestion too. So this feels in reasonable shape to me now.

I also added a release note to flag the potential config issue, and rebased against master.

@purcell purcell marked this pull request as ready for review January 1, 2024 18:07
@purcell
Copy link
Member Author

purcell commented Jan 1, 2024

One thing I decided not to do here was renaming the main executable we produce from NzbDrone to sonarr — the (historical) NzbDrone name is still used in the config path in the service definition, and I didn't want to risk breaking that.

@dacioromero
Copy link
Contributor

@purcell Thanks for getting on this! I just tried importing my v4 backup from an OCI container I was running and it seems to have mostly worked except it failed to restart. Restarting the service manually seems to fix it.

Jan 01 17:12:21 fiyarr NzbDrone[74440]: [Info] LifecycleService: Restart requested.
Jan 01 17:12:21 fiyarr NzbDrone[74543]: The command could not be loaded, possibly because:
Jan 01 17:12:21 fiyarr NzbDrone[74543]:   * You intended to execute a .NET application:
Jan 01 17:12:21 fiyarr NzbDrone[74543]:       The application '/data=/var/lib/sonarr/.config/NzbDrone' does not exist.
Jan 01 17:12:21 fiyarr NzbDrone[74543]:   * You intended to execute a .NET SDK command:
Jan 01 17:12:21 fiyarr NzbDrone[74543]:       No .NET SDKs were found.
Jan 01 17:12:21 fiyarr NzbDrone[74543]: Download a .NET SDK:
Jan 01 17:12:21 fiyarr NzbDrone[74543]: https://aka.ms/dotnet-download
Jan 01 17:12:21 fiyarr NzbDrone[74543]: Learn about SDK resolution:
Jan 01 17:12:21 fiyarr NzbDrone[74543]: https://aka.ms/dotnet/sdk-not-found
Jan 01 17:12:21 fiyarr systemd[1]: sonarr.service: Deactivated successfully.
Jan 01 17:12:21 fiyarr systemd[1]: sonarr.service: Consumed 5.630s CPU time, received 3.3M IP traffic, sent 69.5K IP traffic.

Migrating from v3 to v4 also seems to work. I reverted to v3, restored my v3 backup, and then upgraded and all my series were still there.

This is how I am testing v4

  services.sonarr =
    let
      sonarr4 = import
        (builtins.fetchTarball {
          url = "https://github.com/purcell/nixpkgs/archive/16d725c6f48995ed905d0f70adf7f9a9a4445415.tar.gz";
          sha256 = "02daj2xgq9k9mp7s3xdh7p6m0i4l8i3id7i7kxzgpfh9r8h6h3jl";
        })
        {
          inherit (config.nixpkgs) config;
          system = "x86_64-linux";
        };
      package = sonarr4.sonarr;
    in
    {
      enable = true;
      openFirewall = true;
      user = "media";
      group = "media";
      inherit package;
    };

@purcell
Copy link
Member Author

purcell commented Jan 2, 2024

Thanks for the extra data point, @dacioromero.

Seems to still be happily running for me without issues, so I think this is ready for review and potential merge.

@purcell purcell requested a review from justinas January 3, 2024 08:32
@purcell
Copy link
Member Author

purcell commented Jan 5, 2024

@Sorixelle That's great, thanks so much for sharing! My suggestion would be that we merge this PR first, and then as one of the maintainers I'll definitely pursue getting the source build working based on your code.

@purcell purcell added the needs_merger (old Marvin label, do not use) label Jan 7, 2024
@purcell
Copy link
Member Author

purcell commented Jan 7, 2024

Resolved conflict in the changelog, still ready for merge.

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 7, 2024
@tie
Copy link
Member

tie commented Jan 8, 2024

Since Sonarr v4 should be a drop-in replacement for Sonarr v3, perhaps we can keep the current v3 sonarr package and instead introduce sonarr_4 with backport to 23.11 release? And for 24.05, rename sonarr_4 back to sonarr. This would allow upgrading to Sonarr v4 before 24.05 release, e.g. by setting services.sonarr.package = pkgs.sonarr_4 for people on the stable 23.11 release.

My suggestion would be that we merge this PR first, and then as one of the maintainers I'll definitely pursue getting the source build working based on your code.

FWIW here is my take on source build: https://github.com/tie-infra/nix-config/tree/5db5a22c753c4b15549c750f75802c0049ab5671/parts/sonarr-v4
It needs an update script and there are two minor workarounds for issues in the underlying Nix infrastructure for .NET, but those should be trivial to fix.

pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
@purcell
Copy link
Member Author

purcell commented Jan 8, 2024

FWIW here is my take on source build

Thanks @tie: using your code the build failed for me locally (aarch64-darwin, sandboxed). 😔 https://gist.github.com/purcell/62d4bb42681e560a39035454f9f04243

@purcell
Copy link
Member Author

purcell commented Jan 8, 2024

Since Sonarr v4 should be a drop-in replacement for Sonarr v3, perhaps we can keep the current v3 sonarr package and instead introduce sonarr_4 with backport to 23.11 release?

Maybe? It's probably not for me to say, as a somewhat casual maintainer. It sounds reasonable but I am unfamiliar with the processes, conventions and logistics needed to make it happen.

@tie
Copy link
Member

tie commented Jan 8, 2024

using your code the build failed for me locally (aarch64-darwin, sandboxed). 😔

Uh, I think it would be easier to disable checks on Darwin, tie-infra/nix-config@b94250d
Builds fine for me on aarch64-darwin after this change.

@tie
Copy link
Member

tie commented Jan 9, 2024

Maybe? It's probably not for me to say, as a somewhat casual maintainer. It sounds reasonable but I am unfamiliar with the processes, conventions and logistics needed to make it happen.

In general, as long as the change is acceptable for releases (as in CONTRIBUTING.md), tagging the PR with backport release-YY.MM label is enough, and the bot then creates backport PR (that is merged separately). In this case, adding sonarr_4 package while keeping v3 as sonarr (and potentially sonarr_3) wouldn’t be a breaking change.


Regarding source build, I took some time to implement fixes for buildDotnetModule and I don’t think we can reasonably backport those, and I wouldn’t feel comfortable backporting a package with workarounds (or a package that can’t be cross-compiled). See also staging...tie:nixpkgs:sonarr-v4

@purcell
Copy link
Member Author

purcell commented Jan 13, 2024

In general, as long as the change is acceptable for releases (as in CONTRIBUTING.md), tagging the PR with backport release-YY.MM label is enough, and the bot then creates backport PR (that is merged separately). In this case, adding sonarr_4 package while keeping v3 as sonarr (and potentially sonarr_3) wouldn’t be a breaking change.

This would be the first time such a process has been used for either sonarr or its sibling radarr, and I worry that we're overcomplicating things by suggesting it tbh. End-user functionality and config for this release are more or less unchanged, so it probably qualifies for release broadly as there are no intentional breaking changes.

The only reason folks are suggesting the _3 -> _4 process is the report of broken automatic config migrations, but this is not something that end users can really predict or adjust for, and it apparently only affected a small minority of people with very old configs that had been incrementally upgraded over multiple versions.

If the latter is sufficient to be considered a breaking change, and the release note isn't enough, I would personally suggest we not backport this rather than do the _3 -> _4 dance, as the existing release version still works fine despite being a year out of date.

@purcell
Copy link
Member Author

purcell commented Jan 22, 2024

How to move this forward? I'm a maintainer for this derivation, but I don't have the commit bit. What buy-in is necessary for the change to get merged in its current form, or who decides what must or should be done here?

@eyJhb
Copy link
Member

eyJhb commented Jan 31, 2024

I've been using this patch for some time now, and I don't think we should wait to merge this. Generally the move from mono to dotnet provides many benefits, one of them (and the reason I've started to use this patch), is because of IPv6 support.

Adding it to the release notes should suffice, but, I'm not sure if we can get any higher power to look into this.

@purcell
Copy link
Member Author

purcell commented Feb 5, 2024

Rebased to resolve conflict in release notes.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/3389

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

especially the version re-use must be fixed before this can be merged

pkgs/development/libraries/linenoise/default.nix Outdated Show resolved Hide resolved
pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
pkgs/servers/sonarr/default.nix Outdated Show resolved Hide resolved
Also switch to using assets attached to GitHub releases, which now
seems to be the official download location.
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

diff LGTM, didn't try myself but I want to free the author out of his waiting 🙈

@SuperSandro2000 SuperSandro2000 merged commit a53f27e into NixOS:master Feb 5, 2024
22 of 24 checks passed
@purcell
Copy link
Member Author

purcell commented Feb 5, 2024

Thanks @SuperSandro2000! ❤️

@purcell purcell deleted the sonarr-4 branch February 20, 2024 17:04
@tie tie mentioned this pull request Feb 26, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes needs_merger (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.