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

mpv: use wrapper for scripts and add mpris script #43632

Merged
merged 1 commit into from Jul 19, 2018
Merged

mpv: use wrapper for scripts and add mpris script #43632

merged 1 commit into from Jul 19, 2018

Conversation

jfrankenau
Copy link
Member

Motivation for this change

#14040 added the ability to use Lua scripts with mpv. mpv also supports C scripts/plugins which need to be compiled with libmpv. Thus loading C plugins is not possible by simply modifiying mpv's derivation. I've add a wrapper called mpv-with-scripts to solve this problem.

@Profpatsch, could you have a look as the author of #14040?

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)
  • Fits CONTRIBUTING.md.

@Profpatsch
Copy link
Member

Will take a look later today. Ping me if I forget. :)

@Profpatsch
Copy link
Member

Nice touch with the wrapper script.

I wonder if we should make mpv-with-scripts into a function instead? mpv.WithScripts [ mpvScripts.mpris ]

@Profpatsch
Copy link
Member

Have you tried it with both scripts? The manpage is a lie, it throws an error if you separate the scripts with a comma. It seems to work when you give it multiple --script options. Maybe open an upstream bug request?

@jfrankenau
Copy link
Member Author

Multiple scripts in a single --script don't work for me either. I will file an upstream bug. Also, was there a --scripts with an s option before? I saw this one in your original PR but it doesn't work at all now.

About mpv.WithScripts I don't know what the preferred/better way is. I've done some grepping on the repo and using a seperate attribute seemed more common to me.

@Profpatsch
Copy link
Member

About mpv.WithScripts I don't know what the preferred/better way is. I've done some grepping on the repo and using a seperate attribute seemed more common to me.

Yeah, let’s keep it as it is for now.

Works for me, LGTM!

@Profpatsch Profpatsch merged commit 402ee4e into NixOS:master Jul 19, 2018
@jfrankenau jfrankenau deleted the init-mpv-script-mpris branch July 19, 2018 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jfrankenau @Profpatsch @GrahamcOfBorg