mpv: don't use generated luasocket

[cript0nauta]

The luarocks version of luasocket crashes if the host program has
symbols with the same name as luasocket, such as "socket_create".

This change makes mpv use a custom luasocket derivation that isn't built
from luarocks. This new derivation has the same code as
"luaPackages.luasocket" before commit
274715c.

Closes #88584

Motivation for this change

mpv compiled with samba support crashed when using a luasocket script. See #88584. This PR fixes the issue without disabling samba.

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.

[doronbehar]

@cript0nauta Are you able to explain why does the version we ship is not good enough? I mean, what has changed between the two that made the version you suggest here better?

I noticed you use buildLuaPackage and not buildLuarocksPackage. The later is not documented for as far as I know, and the differences are subtle and hard to explain. I suspect that this is why the derivation you suggest to use here works, not because it's an unstable release.

This PR reminds me of #80528 . Although it seemed promising, @teto opposed to add a secondary version of the same Luarocks package to the collection. I'm afraid I tend to agree with them, because the changes we suggest fix the issue at hand but they add up to the maintenance burden.

[cript0nauta]

Are you able to explain why does the version we ship is not good enough?

Unfortunately, I haven't determined the root cause of the issue yet. What I do know is that when mpv uses the luarocks version of luasocket, it will crash when a script tries to listen on a socket. This happens in Arch Linux too, and only when I'm using luarocks. So my guess is that the bug is either mpv's, luasocket's or luarocks' fault. I don't think this is a problem of buildLuarocksPackage itself.

This PR reminds me of #80528 . Although it seemed promising, @teto opposed to add a secondary version of the same Luarocks package to the collection. I'm afraid I tend to agree with them, because the changes we suggest fix the issue at hand but they add up to the maintenance burden.

Fair enough. I understand having duplicated luasocket versions could be counterproductive. But I don't have a better idea of how could be #88584 fixed without disabling samba support (which I don't use nor consider important, but I guess some users might).

I'll keep investigating the issue to see if I can fix the root cause of the bug instead of doing this duplicated versions hack, but I'm not sure if I'm going to accomplish something.

[doronbehar]

Fair enough. I understand having duplicated luasocket versions could be counterproductive. But I don't have a better idea of how could be #88584 fixed

I do see a way, but it'll require rewriting a lot of the Lua modules ecosystem and it's orthogonal to #85103 and #56398 ...

[teto]

if the luarocks version of luasocket is too old, can't they do another release ? alternatively we could switch to a fork on luarocks since it's clearer to swap sources than having 2 luasockets in nixpkgs.

[doronbehar]

@teto this is not about the version being used, (I don't know why @cript0nauta chose to use the -git suffix), See:

https://github.com/cript0nauta/nixpkgs/blob/f9bcddfaaa50e43a9f027ac63d86383142e40015/pkgs/development/lua-modules/generated-packages.nix#L1168

https://github.com/cript0nauta/nixpkgs/blob/f9bcddfaaa50e43a9f027ac63d86383142e40015/pkgs/top-level/lua-packages.nix#L102-L109

What makes the difference is the use of buildLuaPackage vs buildLuarocksPackage. We've been through a related nightmare at #80528 (comment) .

What I pick out of this, is the fact we are facing issues with some Luarocks modules that don't play nice with luarocks make and we are unable to replace their builder with an alternative, because then these modules are not detected available modules when used as deps for other Lua packages (nvim-client in the case of #80528 ).

In #85103 I proposed to write a common interface for all languages that will allow to handle the discovery of languages' modules solely via environmental variables and Nix attributes - without relying on what luarocks install does (as explained here).

#85103 will have to go through an RFC and all that... and unfortunately I won't have time to work on it until the summer. In the meantime, I'm adding this issue / PR to the list of issues declarative wrappers will be able to solve.

[teto]

I had a look at the rockspec and contrary to luv's case, it doesn't seem to need any specific config.
In principle I don't mind reverting the generated ones until we have declarative wrappers (that should solve many things: luv's is different there was no clear win) but I would like to understand what's the difference to possibly solve the generated one instead
@crypt0nauta is it possible that you diff both folders (generated vs manual) or try to investigate the issue ? you can override parts of the generated plugin in ./pkgs/development/lua-modules/overrides.nix if that helps

[cript0nauta]

Apparently mpv won't use samba anymore: mpv-player/mpv@3b8b7cb. This commit is in master but not released yet. This means the issue I reported will go away in the next mpv version.

I still want to know why this happens so I'll keep investigating. But at least I know in a few weeks (based on mpv's relased cycle) I won't be experiencing this bug and there won't be a duplicated luasocket package in nixpkgs!

[cript0nauta]

I finally discovered the root cause of the bug! It turns out that the github version of luasocket is compiled with -fvisibility=hidden but the luarocks version is not. This gcc flag fixes the issue of having duplicated symbols. This was a known problem of luasocket (see lunarmodules/luasocket#139) that I understand will be solved in the next luasocket release.

I'm closing this PR as I'll open another one disabling samba suport in mpv (and also packaging simple-mpv-webui that was the reason I found the issue).

[doronbehar]

Awesome 👏 .

[teto]

makes sense considering your bug report xD thanks for having looked into it.