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

plexamp: init #85408

Merged
merged 3 commits into from
Jul 31, 2020
Merged

plexamp: init #85408

merged 3 commits into from
Jul 31, 2020
+41 −0

Conversation

killercup
Copy link
Member

@killercup killercup commented Apr 16, 2020 •
edited
Loading

Motivation for this change

Brand new rewrite of the standalone Plex music player (announcment).
Requires a subscription right now.

The app image extraction works and the app starts, but I've not been able to log in. The sign in page opens in Web (even though my default browser is Firefox), which is not able to load any page for some reason. Just wanted to throw this out there for others to test and maybe fix my stupid mistake :)

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 8.has: package (new) This PR adds a new package 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 Apr 16, 2020
@killercup
plexamp: init at 3.0.3
bbe0b5e 
Desktop file does not work.
@afontaine
Copy link
Contributor

@killercup I wholesale copied this into my home-manager configuration (bumping the version to latest, 3.1.1 at time of writing), and it seems to be working flawlessly.

I wonder if there's something in the version bump that changed things?

@killercup @afontaine
plexamp: 3.0.3 -> 3.1.1
b7e1e4c 
Co-authored-by: Andrew Fontaine <andrew@afontaine.ca>
@killercup killercup marked this pull request as ready for review July 21, 2020 11:52
@killercup
Copy link
Member Author

@afontaine thanks! I'm not sure what blocked me from pushing this as a non-draft. Does the desktop file with the icon show up correctly?

@afontaine
Copy link
Contributor

afontaine commented Jul 21, 2020 •
edited
Loading

@killercup I just checked with rofi, and the icon appears to be missing. I also seem to be having intermittent issues building the package as appimageTools.defaultFhsEnvArgs.multiPkgs pkgs pulls in gst_all_1.gtreamer which is broken.

This seems fixed in #93398 but that isn't merged in yet 😞

@killercup
Copy link
Member Author

Thanks for checking! I'll follow that issue and see if I can find some time to update this PR when it's fixed.

You're also very welcome to take over if you have some more time :)

@afontaine
Copy link
Contributor

good news

@killercup it looks like that PR has been merged 😁 I might be able to find some time evening EDT to check things out as well and see if I can get the icon working.

@afontaine
Copy link
Contributor

@killercup found some time to play with it earlier than I thought, got the icon working 👍

@killercup @afontaine
plexamp: fix icon
f80df2f 
Co-authored-by: Andrew Fontaine <andrew@afontaine.ca>
@killercup
Copy link
Member Author

Awesome, thanks so much, Andrew!

This is now ready to be merged.

@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-already-reviewed/2617/190

jonringer
jonringer reviewed Jul 31, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

the app launches fine for me, but I'm unable to do anything after signing in :(

@afontaine
Copy link
Contributor

@jonringer were you able to log in or did it not get pasted the initial sign-in? I noticed it took a few attempts to log in, but it also took a few attempts on my Windows installation as well, so I am not sure if it's an issue with nix or with plexamp 🤔

@jonringer
Copy link
Contributor

jonringer commented Jul 31, 2020 •
edited
Loading

but it also took a few attempts on my Windows installation as well, so I am not sure if it's an issue with nix or with plexamp

Ah, it's a feature. Didn't realise :) /s

Edit: Finally got a different error message. I don't have a plex pass :)

jonringer
jonringer approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer 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
gui launches fine

@jonringer jonringer merged commit cda0a60 into NixOS:master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afontaine afontaine

@jonringer jonringer

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 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
@killercup @afontaine @nixos-discourse @jonringer