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

Allow mpv to use youtube-dl provided at runtime #88136

Closed
wants to merge 1 commit into from
Closed

Conversation

@pstch
Copy link
Contributor

pstch commented May 19, 2020

This change the mpv wrapper to allow it to use a youtube-dl script provided in the user's PATH. This is necessary because youtube-dl is changing very frequently, and the version in nixpkgs does not always work.

Motivation for this change

The youtube-dl script is changing very frequently to adapt with the integrated sources, and is often broken in nixpkgs. Users may use their own version of the script (for example in ~/bin), but mpv ill not use this script as the youtube-dl PATH is prepended instead of being appended.

Things done

Change the mpv wrapper to make it append (--suffix) the youtube-dl PATH instead of prepending it (--prefix).

I'm not sure if this is necessary for the LUA environment as well. Most packages I've seen in nixpkgs seem to prefer --prefix so I kept it for the LUA environment, but can change it to --suffix as well on request.

  • 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.
This change the `mpv` wrapper to allow it to use a `youtube-dl` script provided in the user's `PATH`. This is necessary because `youtube-dl` is changing very frequently, and the version in `nixpkgs` does not always work.
@Mic92
Mic92 approved these changes May 19, 2020
@pstch pstch marked this pull request as ready for review May 19, 2020
@pstch
Copy link
Contributor Author

pstch commented May 19, 2020

Another possibility would be to change the package code to allow using .override { youtube-dl = null; }, so that we don't allow leaking in youtube-dl by default. Currently this override does not work as it makes an assertion fail.

I'm not sure what's the best solution.

@jtojnar
Copy link
Contributor

jtojnar commented May 19, 2020

Does not .override { youtubeSupport = false; } work? I would expect it to be just picked up from user path if not in wrapper.

@pstch
Copy link
Contributor Author

pstch commented May 19, 2020

@jtojnar Oh yes sorry, that works as well (I tested it). It has the disadvantage (like the override) that it makes any user that want to provide his own youtube-dl recompile mpv, but it still may be a better solution than allowing youtube-dl to leak from the environment. Not sure what's the best course here.

@Mic92
Copy link
Contributor

Mic92 commented May 19, 2020

I would say that having a newer youtube-dl might be more important to users rather than the leakage. We also do not bundle make with a c compiler from the same reason.

@jtojnar
Copy link
Contributor

jtojnar commented May 19, 2020

On the other hand, if user uses mpv from unstable and has youtube-dl installed, it can lead to confusion as well.

The runtime dependencies can also be added in a separate wrapper derivation to avoid rebuilds.

@Mic92
Copy link
Contributor

Mic92 commented May 19, 2020

Yeah maybe we should just backport all youtube-dl updates because of the service breakages.

@AndersonTorres
Copy link
Member

AndersonTorres commented May 21, 2020

My ~/local/bin/youtube-dl can take precedency above /nix/store/<crazy-hash>-youtube-dl-<version> without problems?

@Mic92
Copy link
Contributor

Mic92 commented May 21, 2020

My ~/local/bin/youtube-dl can take precedency above /nix/store/<crazy-hash>-youtube-dl-<version> without problems?

Some people consider this at the problem. For example your local youtube-dl could be incompatible for some reason with mpv, while the one bundled with nixpkgs is hopefully not as it is the version everyone tests mpv + youtube-dl with.

@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

I had a similar argument a while ago at #85640 . My personal opinion, is that specifically for $PATH, if a program does nothing more then use a certain executable from $PATH and that executable is not needed for the most basic functionality of the program, then it's easier for most people if it'll use the $PATH of the calling environment as expected by most people and as it is in every other "normal" distro.

The idea is that I think most users expect their installations to have an effect on the usage of an executable by another program. Not to mention recompilation.

Hence I'd vote for even removing any $PATH modifications in the wrapper, but as this will be incompatible with what some users' usage of this derivation, the least we can do is to use --suffix and not --prefix.

@jtojnar
Copy link
Contributor

jtojnar commented May 22, 2020

The idea is that I think most users expect their installations to have an effect on the usage of an executable by another program. Not to mention recompilation.

One of the goals of Nix is determinism. This expectation goes against it and so it is wrong. IMHO having packages take dependencies from the environment is harmful since it strengthens this incorrect expectation.

Of course, it is not feasible to have fully deterministic dependencies in all cases but here it is rather easy so we should do it.

Hence I'd vote for even removing any $PATH modifications in the wrapper, but as this will be incompatible with what some users' usage of this derivation, the least we can do is to use --suffix and not --prefix.

And now when youtube-dl changes API and people will get hard to diagnose mismatches. Using packages from single nixpkgs commit guarantees compatibility (modulo bugs that should have been caught in review); taking things from PATH, all bets are off.

@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

One of the goals of Nix is determinism.

I'm up for determinism. But, due to the nature of the way wrappers are currently created in Nixpkgs, overriding a wrapper unfortunately requires rebuilding the whole package. With something like #85103, users will be able to easily override wrappers as the wrappers themselves will be a separate derivation that doesn't build anything.

There is a hacky option which I'm thinking of and I'm using this approach for other packages in my personal overlays, with which a user could override the wrapper of mpv without building, (not tested):

self: super:

{
  mpv-with-scripts = (super.mpv-with-scripts.override {
    scripts = [ ];
  }).overrideAttrs(oldAttrs: {
    postBuild = oldAttrs.postBuild + ''
      cp -L $out/bin/.mpv-wrapped_ $out/bin/.mpv-wrapped_.tmp
      sed 's,${self.youtube-dl},/path/to/my/updated/youtube-dl,g' $out/bin/.mpv-wrapped_.tmp
      mv -f $out/bin/.mpv-wrapped_.tmp $out/bin/.mpv-wrapped_
    '';
  });
}

But that's super ugly.

@AndersonTorres
Copy link
Member

AndersonTorres commented May 22, 2020

@doronbehar

if a program does nothing more then use a certain executable from $PATH and that executable is not needed for the most basic functionality of the program, then it's easier for most people if it'll use the $PATH of the calling environment as expected by most people and as it is in every other "normal" distro.

Well, I think NixOS is not a "normal distro". Please, don't take it harshly (I am a bit bad conveying the right emotions in my mother tongue, how much more a foreign one!).

As the guys above said, specially @jtojnar, NixOS is deterministic - or, in other more nuanced words, you have a very huge burden of proof if you want to implement something that goes against determinism.

If some thing changes the behavior of another thing, then the relationship between these two things is to be made explicit, let it be by us Nixpgs maintainers or by the user (via configuration files, command-line options, even environment variables or whatever).

The idea is that I think most users expect their installations to have an effect on the usage of an executable by another program.

I very don't like an elitistic way to think about "user's necessities" here. But I don't think the users really know what they want.

A thing that makes me choose NixOS as my main (and only) distribution is the fact I know that a single configuration file specifies all my system and I don't need to worry about unexpected behaviors like "installing a kernel patch will break video driver". Thinking in terms of "structured programming", NixOS doesn't hide states in global variables.

And your solution makes youtube-dla "global variable" to an otherwise working mpv.

Not to mention recompilation.

Seriously, I would prefer recompilation if the other option is state leaking. MPV is not a huge software like, say, Libreoffice or X11, and it doesn't have huge dependencies as a full Rust environment that isn't always pre-installed.

Nonetheless, I recognize the value of your consideration. It isn't elegant to recompile mpv just because we change youtube-dl, because youtube-dl is not a build-time dependency of mpv, but just a runtime one (or at least it is what I conclude by looking at it). In a certain sense, it points a "flaw" in the way we are packaging mpv.

Fortunately, you have pointed out a better solution: an external wrapper. I am all in your idea now I am reading it!

Hence I'd vote for even removing any $PATH modifications in the wrapper, but as this will be incompatible with what some users' usage of this derivation

As I have said, I will take the side of us Nixpkgs. Removing $PATH modifications in the wrapper looks way better.

@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

Fortunately, you have pointed out a better solution: an external wrapper. I am all in your idea now I am reading it!

If everyone are up for "external" wrappers, then perhaps: #88620 .

@doronbehar doronbehar mentioned this pull request May 23, 2020
0 of 10 tasks complete
@AndersonTorres
Copy link
Member

AndersonTorres commented May 23, 2020

@doronbehar , do you think we can close this PR and approve the other one, then?

@doronbehar
Copy link
Contributor

doronbehar commented May 23, 2020

Yes, I think @pstch is also up for this idea.

@pstch
Copy link
Contributor Author

pstch commented May 23, 2020

Yes, I agree that the other PR is a better solution for the reasons stated in this thread.

Thank you @doronbehar for providing a better solution.

@pstch pstch closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.