Skip to content

roon-server: revamp #93715

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
Merged
merged 2 commits into from
Jul 24, 2020
Merged

roon-server: revamp #93715

merged 2 commits into from
Jul 24, 2020
+55 −52

Conversation

lovesegfault
Copy link
Member

@lovesegfault lovesegfault commented Jul 23, 2020 •
edited
Loading

Motivation for this change

This builds on #92945 by completely revamping the roon-server derivation, and in the process fixing numerous issues with it.

With these changes I now have a fully functioning Roon on NixOS setup for the first time!

cc. @eljojo
cc. @flokli @andir

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 nixpkgs-review --run "nixpkgs-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.

Sorry, something went wrong.

@ofborg ofborg 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/` 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 23, 2020
@lovesegfault lovesegfault requested a review from Ekleog July 23, 2020 20:11
Ekleog
Ekleog approved these changes Jul 24, 2020
View reviewed changes
pkgs/servers/roon-server/default.nix

installPhase = ''
runHook preInstall
Copy link
Member

Choose a reason for hiding this comment

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

I'd bet the runHooks aren't actually required for proper operation... are they?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not, but because I'm overriding the whole phase I thought I'd keep them.

@Ekleog Ekleog merged commit 0c075ce into NixOS:master Jul 24, 2020
@lovesegfault lovesegfault deleted the roon-server-revamp branch July 24, 2020 18:17
@vcunat
Copy link
Member

vcunat commented Jul 26, 2020

The zlib hack from #92945 isn't required anymore, right?

@vcunat vcunat mentioned this pull request Jul 26, 2020
Closed
10 tasks
@Ekleog
Copy link
Member

Ekleog commented Jul 27, 2020

As far as I understand this is taken care of by the patchelf hook — that said I must say I am not a user of roon myself, so couldn't confirm with 100% certainty.

@eljojo
Copy link

eljojo commented Jul 27, 2020 •
edited
Loading

Hi everyone, I'm glad that the roon-server package is getting more attention.

I have to admit it makes me feel a bit sad that this PR was merged before mine, I worked really hard on it and was hoping it'd get merged.

With that being said, would someone be able to explain to me:

  • how this goes about solving the zlib issue?
    • I went through the diff but still can't understand how this fixes the issue
    • I am not doubting that this does indeed fix it, I just want to improve my understanding of nix
    • @Ekleog mentioned the patchelf hook but that was already running before?
  • any guidance on how to run this on my local NixOS?
    • it seems superior than what I already have running, but I am not sure how to point my local NixOS to the "master branch" version of this roon-server package.

Thanks for your help and doing this work

@vcunat
Copy link
Member

vcunat commented Jul 28, 2020

any guidance on how to run this on my local NixOS?

There are various ways, e.g. use parameter -I nixpkgs=/path (accepted by nixos-rebuild, nix-* and even new nix), where /path can even be like https://github.com/nixos/nixpkgs/archive/master.tar.gz

@lovesegfault
Copy link
Member Author

@vcunat It's being handled by the autoPatchelfHook.

@eljojo

how this goes about solving the zlib issue?

The original Nix code was a bit "traditional" in that it manually tried to patchelf binaries, and so missed patchelf-ing zlib into the mono runtime. My changes uses the autoPatchelfHook as opposed to manually calling patchelf. What this effectively does is scan all ELF binaries for libraries and automatically patchelf the correct rpaths in. Thus not only the zlib issue you had is fixed, but also the alsalib issue I had, and any other new dynlinked dependencies Roon might add in the future.

any guidance on how to run this on my local NixOS?

You can add it in an overlay, but i think you'd benefit from going to #nixos on freenode for more interactive help on how to set those things up

@eljojo
Copy link

eljojo commented Jul 28, 2020

👍 thanks for the context this is super useful!

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

@Ekleog Ekleog

@doronbehar doronbehar

@flokli flokli

@andir andir

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: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lovesegfault @vcunat @Ekleog @eljojo