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.

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

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

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.

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.

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

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

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.

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.

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.

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.

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

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 , do you think we can close this PR and approve the other one, then?

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

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.