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

Jump to bottom

plex: rewrite to use FHS userenv #56565

Merged
merged 1 commit into from Apr 24, 2019
Merged

plex: rewrite to use FHS userenv #56565

merged 1 commit into from Apr 24, 2019

Conversation

andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Mar 1, 2019 •
edited

Motivation for this change

This PR attempts to rewrite the Plex derivation and NixOS module to use buildFHSUserEnv. Plex downloads binary codecs from the internet and runs them, and this prevents errors when that happens. This PR is an alternative to #48506 that maintains compatibility with the previous Plex dataDir option.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Remaining questions
  • We now default to a dataDir in $HOME if none is given to the FHS userenv, since that feels less likely to break without running as elevated permisisons (though the NixOS module keeps the previous default). Does this make sense?
  • Is there a nicer way to do the plex / plexRaw split that still allows overriding plexRaw.src for Plex Pass users?
  • Does this sound sensible?

cc @yegortimoshenko @colemickens @dywedir @Forkk @LnL7 @pjones @thoughtpolice

@andrew-d andrew-d requested a review from infinisil as a code owner March 1, 2019 06:16
@pjones
Copy link
Contributor

pjones commented Mar 2, 2019

This is probably the way to go with Plex. To use any of the Plex Pass features I had to create an Ubuntu VM. I'd rather run it in a NixOS container and this looks like a move in the right direction.

@colemickens
Copy link
Member

This looks good to me and works with my existing library. Thank you for taking this over, I am excited to see this merged!

@benley
Copy link
Member

benley commented Mar 4, 2019

I'll wait for one of the actual package maintainers to chime in before merging, but this looks pretty good to me.

bachp
bachp requested changes Mar 15, 2019
View reviewed changes
Copy link
Member

@bachp bachp left a comment

Choose a reason for hiding this comment

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

I tried to run this against the current nixos-unstable-small (eb2a26f5c61) channel.

The service won't start:

Mar 15 22:39:37 odin systemd[1]: Started Plex Media Server.
Mar 15 22:39:37 odin plexmediaserver[29398]: /nix/store/cinw572b38aln37glr0zb8lxwrgaffl4-bash-4.4-p23/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
Mar 15 22:39:37 odin plexmediaserver[29398]: terminate called after throwing an instance of 'std::runtime_error'
Mar 15 22:39:37 odin plexmediaserver[29398]:   what():  locale::facet::_S_create_c_locale name not valid
Mar 15 22:39:37 odin systemd[1]: plex.service: Main process exited, code=killed, status=6/ABRT
Mar 15 22:39:37 odin systemd[1]: plex.service: Failed with result 'signal'.
Mar 15 22:39:38 odin systemd[1]: plex.service: Service RestartSec=100ms expired, scheduling restart.
Mar 15 22:39:38 odin systemd[1]: plex.service: Scheduled restart job, restart counter is at 5.
Mar 15 22:39:38 odin systemd[1]: Stopped Plex Media Server.

I'm not sure this is related to this PR as I got a similar issue with LOCALE while using python 3 in a FHSEnv. #56347 might be related.

@bachp bachp mentioned this pull request Mar 15, 2019
Closed
@andrew-d
Copy link
Contributor Author

andrew-d commented Apr 1, 2019 •
edited

@bachp - Could you pleas try again? I just rebased against master, and #56347 is closed now; I'm running it on 18.09 successfully, but haven't tried against a recent master/staging build.

@bachp
Copy link
Member

bachp commented Apr 1, 2019

@andrew-d The issue still exists. Even after the rebase plex refuses to start with the same error. I think #56347 is not fixed.

@andrew-d
Copy link
Contributor Author

andrew-d commented Apr 2, 2019

@bachp - Aha, I think I figured it out; 467f0f9 is only in staging and not in master, I think. Can you check out this branch (based on master), cherry-pick in 467f0f9, and then try one more time? Sorry for the run-around here!

@bachp bachp mentioned this pull request Apr 2, 2019
Merged
10 tasks
@bachp
Copy link
Member

bachp commented Apr 2, 2019

@andrew-d Unfortunatly just 467f0f9 is not enough. It also requires #58847 to work properly.

@bachp
Copy link
Member

bachp commented Apr 10, 2019 •
edited

I rebased this thogether with all the other commits missing to make it working: https://github.com/bachp/nixpkgs/tree/plexfhs-fhsfixes

@bachp
Copy link
Member

bachp commented Apr 12, 2019

@andrew-d This now works out of the box on master. You can use my rebased version from https://github.com/bachp/nixpkgs/tree/plexfhs-fhsfixes that solves the merge conflict by updating the plex version.

@andrew-d
Copy link
Contributor Author

@bachp - Okay, rebased this on master, which I think should have fixed merge conflicts. Thank you for all your help with the locale issues!

bachp
bachp approved these changes Apr 14, 2019
View reviewed changes
Copy link
Member

@bachp bachp left a comment

Choose a reason for hiding this comment

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

With the latest rebase it works out of the box, even on top os nixos-unstable-small.

@bachp
Copy link
Member

bachp commented Apr 23, 2019

Is there a reson this doesn't get merged?

Rebased version without conflict: https://github.com/bachp/nixpkgs/tree/plexfhs-fhsfixes

@andrew-d
Copy link
Contributor Author

@bachp - Rebased on master again.

@andrew-d andrew-d mentioned this pull request Apr 24, 2019
Closed
10 tasks
@bachp bachp mentioned this pull request Apr 24, 2019
Merged
10 tasks
@elseym
Copy link
Member

elseym commented Apr 24, 2019

i can also confirm that this change works with the current master, thanks @andrew-d and @bachp!

/cc @fpletz

@benley
Copy link
Member

benley commented Apr 24, 2019

Any known remaining blockers to merging this? I'm not sure whose approval we're waiting for here

@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

@GrahamcOfBorg eval
@GrahamcOfBorg build plex

(though I suppose it'll error on non-free software)

@grahamc grahamc merged commit 8570692 into NixOS:master Apr 24, 2019
@andrew-d andrew-d deleted the adunham/plex-fhs branch April 24, 2019 23:38
@andrew-d
Copy link
Contributor Author

@grahamc - Thank you very much; greatly appreciated!

@bachp bachp mentioned this pull request Apr 25, 2019
Closed
@bachp bachp mentioned this pull request May 2, 2019
Closed
9 tasks
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/plex-on-aarch64/13945/1

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

@colemickens colemickens colemickens left review comments

@benley benley benley left review comments

@bachp bachp bachp approved these changes

@infinisil infinisil Awaiting requested review from infinisil

@LnL7 LnL7 Awaiting requested review from LnL7

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees
No one assigned
Labels
6.topic: nixos 8.has: clean-up 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@andrew-d @pjones @colemickens @benley @bachp @elseym @grahamc @nixos-discourse @GrahamcOfBorg