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-mpv-shim: init at 1.7.12 #77884

Merged
merged 2 commits into from Mar 2, 2020
Merged

plex-mpv-shim: init at 1.7.12 #77884

merged 2 commits into from Mar 2, 2020

Conversation

colemickens
Copy link
Member

Tested. Working as best as it can, I think.

(Specifically on an RPi4 with Sway)

  • 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.

jonringer
jonringer reviewed Feb 23, 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 second commit message should include unstable-2020-02-01 as the version, otherwise LGTM

@colemickens colemickens force-pushed the nixpkgs-plex-mpv-shim branch 2 times, most recently from 6d9c4e0 to 1e9c517 Compare February 23, 2020 22:41
@colemickens
Copy link
Member Author

@jonringer thanks! Updated the commit message to the correct format, (and bumped the unstable package to the latest commit).

@colemickens colemickens changed the title plex-mpv-shim: init at 1.7.0 plex-mpv-shim: init at 1.7.12 Feb 24, 2020
@colemickens
Copy link
Member Author

Update plex-mpv-shim itself to latest too. It work quite well now! Should be good to merge.

jonringer
jonringer reviewed Feb 25, 2020
View reviewed changes
pkgs/development/python-modules/mpv-jsonipc/default.nix Outdated

buildPythonPackage rec {
pname = "mpv-jsonipc";
version = "unstable-2020-02-19";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version = "unstable-2020-02-19";
version = "unstable-2020-02-19";
disabled = pythonOlder "3.6";

to avoid:

  Processing ./python_mpv_jsonipc-1.1.5-py2-none-any.whl
  ERROR: Package 'python-mpv-jsonipc' requires a different Python: 2.7.17 not in '>=3.6'
cannot build derivation '/nix/store/y6499z8wxmlqjxpf8dgwim4pylqa9l7w-plex-mpv-shim-1.7.12.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/akz7a2cxndlbmq4k3hkx6y7lj7318dwl-env.drv': 2 dependencies couldn't be built
[3 built (2 failed), 1 copied (0.2 MiB), 0.0 MiB DL]
error: build of '/nix/store/akz7a2cxndlbmq4k3hkx6y7lj7318dwl-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/77884
2 package failed to build:
plex-mpv-shim python27Packages.mpv-jsonipc

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated per your suggestion. Pushed. nix-review pr 77884 passes now.

Question -- I understand why nix review tested the python36 review, but why does it look like yours try to build plex-mpv-shim with python36 whereas I was using plex-mpv-shim happily (aka, it wasn't building with python36 on my machine).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, thanks. nix-review pr 77884 passes now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled it from their assertion: Python: 2.7.17 not in '>=3.6'

@colemickens colemickens force-pushed the nixpkgs-plex-mpv-shim branch 4 times, most recently from 873c56c to 312851d Compare February 26, 2020 23:21
jonringer
jonringer reviewed Feb 26, 2020
View reviewed changes
pkgs/development/python-modules/mpv-jsonipc/default.nix Outdated
, tqdm, websocket_client, pythonOlder }:

buildPythonPackage rec {
pname = "mpv-jsonipc";
Copy link
Contributor

Choose a reason for hiding this comment

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

this should match the pypi name https://pypi.org/project/python-mpv-jsonipc/

Suggested change
pname = "mpv-jsonipc";
pname = "python-mpv-jsonipc";

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize it was being published on pypi. They're not tagging releases on GitHub though. I'm going to file an issue so we can ship a tagged release instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

iwalton3/python-mpv-jsonipc#1

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonringer: I've got the pname change staged here locally, do I need to also rename the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please

pkgs/development/python-modules/mpv-jsonipc/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/plex-mpv-shim/default.nix Outdated Show resolved Hide resolved
@colemickens colemickens mentioned this pull request Feb 27, 2020
Closed
jonringer
jonringer approved these changes Mar 2, 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
commits LGTM

[4 built, 5 copied (16.7 MiB), 3.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/77884
3 package built:
plex-mpv-shim python37Packages.python-mpv-jsonipc python38Packages.python-mpv-jsonipc

@jonringer
Copy link
Contributor

@GrahamcOfBorg build plex-mpv-shim python37Packages.python-mpv-jsonipc python38Packages.python-mpv-jsonipc

@jonringer jonringer merged commit bd25053 into NixOS:master Mar 2, 2020
@jojosch jojosch mentioned this pull request Mar 6, 2020
Merged
10 tasks
This was referenced Mar 25, 2020
Closed
Closed
@matthew-piziak
Copy link
Contributor

When I install and run plex-mpv-shim from unstable, I get the following error:

2020-03-27 14:52:17,744 [    INFO] Loaded settings from json: /home/matt/.config/plex-mpv-shim/conf.json
2020-03-27 14:52:17,744 [ WARNING] Cannot load GUI. Falling back to command line interface.
Traceback (most recent call last):
  File "/nix/store/0w18z9pf1qrq9lkhy7r5f0fhscms4c58-plex-mpv-shim-1.7.12/lib/python3.7/site-packages/plex_mpv_shim/mpv_shim.py", line 39, in main
    from .gui_mgr import userInterface
  File "/nix/store/0w18z9pf1qrq9lkhy7r5f0fhscms4c58-plex-mpv-shim-1.7.12/lib/python3.7/site-packages/plex_mpv_shim/gui_mgr.py", line 1, in <module>
    from pystray import Icon, MenuItem, Menu
ModuleNotFoundError: No module named 'pystray'
2020-03-27 14:52:18,116 [    INFO] Started GDM service
2020-03-27 14:52:18,119 [    INFO] Started HTTP server

@colemickens
Copy link
Member Author

@matthew-piziak Feel free to open a new issue. That should be a non-fatal error. If I get past it, I just hit another with the GUI. I think we can consider the GUI unsupported for now (didn't know it existed), but a new issue for it would be good.

@colemickens
Copy link
Member Author

To clarify what I just wrote, you can ignore that error about pystray. If you ignore it, plex-mpv-shim still works as expected. If I patch plex-mpv-shim to see pystray, it just hits an error about tkinter and gives up on the GUI again.

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

@doronbehar doronbehar doronbehar left review comments

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@colemickens @jonringer @matthew-piziak @doronbehar