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.

Sign up for GitHub

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

Jump to bottom

sonarr: build from source #291640

Merged
merged 2 commits into from
Jul 4, 2024
Merged

sonarr: build from source #291640

merged 2 commits into from
Jul 4, 2024

Conversation

tie
Copy link
Member

@tie tie commented Feb 26, 2024 •
edited
Loading

Description of changes

This change refactors sonarr package (and its update script) to build the package from source. It also contains changes to the underlying .NET build infrastructure required for this package.

See also #278050 (comment)

Important

This change relies on the recent PRs for the .NET build infrastructure (for structured attributes and cross-compilation) and cannot be backported.

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.

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Feb 26, 2024
@ofborg ofborg bot requested a review from purcell February 26, 2024 20:18
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Feb 26, 2024
@bjornfor
Copy link
Contributor

buildDotnetModule: do not run dotnet command using env

Why not? Why was using env added in the first place? (Might be a good idea to answer that in the commit message.)

@tie tie force-pushed the sonarr-v4 branch 2 times, most recently from e89cd3c to 2d4ecd5 Compare February 27, 2024 16:44
@tie
Copy link
Member Author

tie commented Feb 28, 2024

W.r.t .NET build infrastructure updates. Some packages failed to build their web UI likely because of #253156, but otherwise looks good.

Result of nixpkgs-review pr 291640 run on x86_64-linux 1

5 packages failed to build:
  • audiobookshelf
  • jellyfin
  • kavita
  • slskd
  • sonarr
60 packages built:
  • ArchiSteamFarm
  • BeatSaberModManager
  • alttpr-opentracker
  • avalonia-ilspy
  • azure-functions-core-tools
  • boogie (dotnetPackages.Boogie)
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • dafny
  • denaro
  • depotdownloader
  • discordchatexporter-cli
  • eventstore
  • fable
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • git-credential-manager
  • github-runner
  • ilspycmd
  • inklecate
  • jackett
  • knossosnet
  • libation
  • lubelogger
  • marksman
  • mqttmultimeter
  • msbuild
  • naps2
  • nbxplorer
  • netcoredbg
  • networkminer
  • nuget-to-nix
  • omnisharp-roslyn
  • openra
  • opentabletdriver
  • openutau
  • osu-lazer
  • parabolic
  • pbm
  • pinta
  • ps3-disc-dumper
  • pupdate
  • roslyn
  • roslyn-ls
  • ryujinx
  • scarab
  • space-station-14-launcher
  • tests.dotnet.project-references
  • tone
  • torrentstream
  • wasabibackend
  • xivlauncher

@SuperSandro2000
Copy link
Member

Is there a particular reason you are targeting staging here?

@tie
Copy link
Member Author

tie commented Feb 28, 2024

I was expecting more rebuilds from .NET changes 😅

@tie tie changed the base branch from staging to master February 28, 2024 15:03
@tie
Copy link
Member Author

tie commented Feb 28, 2024

Result of nixpkgs-review pr 291640 run on x86_64-linux 1

64 packages built:
  • ArchiSteamFarm
  • BeatSaberModManager
  • alttpr-opentracker
  • audiobookshelf
  • avalonia-ilspy
  • azure-functions-core-tools
  • boogie
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • dafny
  • denaro
  • depotdownloader
  • discordchatexporter-cli
  • eventstore
  • fable
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • git-credential-manager
  • github-runner
  • ilspycmd
  • inklecate
  • jackett
  • jellyfin
  • kavita
  • knossosnet
  • libation
  • lubelogger
  • marksman
  • mqttmultimeter
  • msbuild
  • naps2
  • nbxplorer
  • netcoredbg
  • networkminer
  • nuget-to-nix
  • omnisharp-roslyn
  • openra
  • opentabletdriver
  • openutau
  • osu-lazer
  • parabolic
  • pbm
  • pinta
  • ps3-disc-dumper
  • pupdate
  • roslyn
  • roslyn-ls
  • ryujinx
  • scarab
  • slskd
  • sonarr
  • space-station-14-launcher
  • tone
  • torrentstream
  • wasabibackend
  • xivlauncher

@purcell
Copy link
Member

purcell commented Feb 28, 2024

Not sure I can meaningfully review this, but as one of the sonarr maintainers this is a great step, and the specific changes to the Sonarr derivation look good to me.

@tie tie marked this pull request as ready for review February 28, 2024 16:34
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 2, 2024
@ofborg ofborg bot requested a review from purcell March 2, 2024 12:55
@purcell
Copy link
Member

purcell commented Mar 5, 2024

I tried running this on MacOS but can't execute the resulting Sonarr executable, strangely:

❯ nix run 'github:tie/nixpkgs/sonarr-v4#sonarr'
zsh: killed     nix run 'github:tie/nixpkgs/sonarr-v4#sonarr'

or, more directly,

❯ /nix/store/m7ykc6igjqq9ivnb6rca0qlz8njr68fp-sonarr-4.0.1.929/bin/Sonarr
zsh: killed     /nix/store/m7ykc6igjqq9ivnb6rca0qlz8njr68fp-sonarr-4.0.1.929/bin/Sonarr

and there's a Console message saying

ASP: Security policy would not allow process: 65161, /nix/store/m7ykc6igjqq9ivnb6rca0qlz8njr68fp-sonarr-4.0.1.929/lib/sonarr/Sonarr

In contrast, this correctly starts the server:

❯ nix run 'github:NixOS/nixpkgs/nixpkgs-unstable#sonarr'

Perhaps there's a codesigning step missing for darwin, but I'mn not an expert on such things sorry.

@tie
Copy link
Member Author

tie commented Mar 9, 2024

@purcell, weird, I can’t reproduce this in macOS VM (but then I guess it may have relaxed security constraints). Do other .NET packages work for you (e.g. jellyfin)?

@purcell
Copy link
Member

purcell commented Mar 11, 2024

weird, I can’t reproduce this in macOS VM (but then I guess it may have relaxed security constraints).

I have SIP enabled, which would be the MacOS default, and I have the sandbox enabled too.

Do other .NET packages work for you (e.g. jellyfin)?

Yes, nix run 'github:NixOS/nixpkgs#jellyfin' executes fine.

@tie
Copy link
Member Author

tie commented Mar 14, 2024 •
edited
Loading

@purcell, looks like something related to .NET is actually broken in upstream Nixpkgs, not sure why binary cache is fine though.

System information 
; nix show-config sandbox
true
; git rev-parse HEAD
bd5ddf2c6bfafff031edf80221e1ee94e86ca10a
; git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
; system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: macOS 14.2.1 (23C71)
      Kernel Version: Darwin 23.2.0
      Boot Volume: Macintosh HD
      Boot Mode: Normal
      Computer Name: ryu
      User Name: Ivan Trubach (tie)
      Secure Virtual Memory: Enabled
      System Integrity Protection: Enabled

; nix build --verbose --file . jellyfin
this path will be fetched (10.93 MiB download, 43.05 MiB unpacked):
  /nix/store/znamd3abcaszf0p8xdin9g8y16jk1nsg-jellyfin-10.8.13
copying path '/nix/store/znamd3abcaszf0p8xdin9g8y16jk1nsg-jellyfin-10.8.13' from 'https://cache.nixos.org'...
; ./result/bin/jellyfin --help

Works when jellyfin is fetched from binary cache.

But then

; nix build --file . --rebuild jellyfin
error: derivation '/nix/store/1ljiqchsj7flkgzsc0063n54nb40av9k-jellyfin-10.8.13.drv' may not be deterministic: output '/nix/store/znamd3abcaszf0p8xdin9g8y16jk1nsg-jellyfin-10.8.13' differs

and to confirm:

; nix store delete /nix/store/znamd3abcaszf0p8xdin9g8y16jk1nsg-jellyfin-10.8.13
1 store paths deleted, 43.02 MiB freed
; nix build --file . --offline jellyfin
; ./result/bin/jellyfin 
zsh: killed     ./result/bin/jellyfin

Edit: works fine when rebuilding with sandbox disabled. I guess macOS builders don’t have sandbox enabled? That would be weird but not unexpected.

@purcell
Copy link
Member

purcell commented Mar 14, 2024

Ugh, that's unfortunate. But good investigation skills! Any idea what we could do to escalate this or find out more?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@github-actions github-actions bot added 6.topic: dotnet Language: .NET and removed 8.has: documentation This PR adds or changes documentation labels Jun 18, 2024
@github-actions github-actions bot removed the 6.topic: dotnet Language: .NET label Jun 18, 2024
@tie tie force-pushed the sonarr-v4 branch 3 times, most recently from 8eb5543 to 8c03b29 Compare June 19, 2024 01:32
tie added 2 commits June 19, 2024 04:44
@tie
sonarr: build from source
b7c782f 
This change refactors sonarr package to build from source instead of
downloading pre-built releases.
@tie
nixos/sonarr: use lib.getExe and escape args
5d7d3dc
@tie tie marked this pull request as ready for review June 19, 2024 02:40
@tie
Copy link
Member Author

tie commented Jun 19, 2024

Result of nixpkgs-review pr 291640 run on aarch64-linux 1

1 package blacklisted:
  • nixos-install-tools
1 package built:
  • sonarr

@tie
Copy link
Member Author

tie commented Jun 22, 2024 •
edited
Loading

@purcell, this should be ready for review and without .NET infrastructure changes. Note that these were merged separately (huge thanks to .NET build infrastructure maintainers for fast review cycles) and are required for this PR. That is, Sonarr version updates for the stable release channel would have to be done separately (see also note in the PR description).

@purcell
Copy link
Member

purcell commented Jun 24, 2024

Sounds great to me, and I very much support this, but I'm a bit too slammed currently to spin it up locally for testing.

purcell
purcell reviewed Jun 24, 2024
View reviewed changes
pkgs/by-name/so/sonarr/package.nix
description = "Smart PVR for newsgroup and bittorrent users";
homepage = "https://sonarr.tv";
license = lib.licenses.gpl3Only;
maintainers = with lib.maintainers; [ fadenb purcell tie ];
Copy link
Member

Choose a reason for hiding this comment

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

Thank goodness you're adding yourself as a maintainer for this, because maintaining this new version of the derivation is likely beyond my wizard level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I’m, unfortunately, stuck with Sonarr 😅
It’s sad that it’s not fully self-hostable (it requires skyhook.sonarr.tv to be up and reachable for operation), but there are no other alternatives with comparable feature set, so it’s an important piece of infrastructure for me.

I don’t think there would be any non-trivial maintenance with source build aside from occasionally bumping .NET version, but feel free to ping me if there are any issues (besides, ofborg will ping me anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to help with general dotnet source-build stuff too.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 26, 2024
@corngood
Copy link
Contributor

corngood commented Jul 4, 2024

Sounds great to me, and I very much support this, but I'm a bit too slammed currently to spin it up locally for testing.

@purcell I'd like your approval before merging. Should we wait for you to test it?

purcell
purcell approved these changes Jul 4, 2024
View reviewed changes
Copy link
Member

@purcell purcell left a comment

Choose a reason for hiding this comment

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

I built and ran this without issues on aarch64-darwin, and given the state of the CI for this PR, I'm going to approve it and suggest we merge.

@corngood corngood merged commit b44b62d into NixOS:master Jul 4, 2024
32 checks passed
@tie tie deleted the sonarr-v4 branch July 4, 2024 15:11
@purcell
Copy link
Member

purcell commented Jul 4, 2024

Thanks @tie! (and @corngood for the merge)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambroisie ambroisie ambroisie left review comments

@GGG-KILLER GGG-KILLER GGG-KILLER left review comments

@corngood corngood corngood approved these changes

@purcell purcell purcell approved these changes

@IvarWithoutBones IvarWithoutBones Awaiting requested review from IvarWithoutBones

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tie @bjornfor @SuperSandro2000 @purcell @corngood @GGG-KILLER @ambroisie @wegank