luajit: adjust defaults LUA_(C)PATH

[teto]

Running in an ubuntu, I get:

root@a5319686430d:/# luajit -e "print(package.path)" ./?.lua;/usr/share/luajit-2.1.0-beta3/?.lua;/usr/local/share/lua/5.1/?.lua;/usr/local/share/lua/5.1/?/init.lua;/usr/share/lua/5.1/?.lua;/usr/share/lua/5.1/?/init.lua 
root@a5319686430d:/# luajit -e "print(package.cpath)" ./?.so;/usr/local/lib/lua/5.1/?.so;/usr/lib/x86_64-linux-gnu/lua/5.1/?.so;/usr/local/lib/lua/5.1/loadall.so

so I've adjusted the paths accordingly

TODO target staging

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[teto]

I havent pushed my latest changes so not to override the CI but turns out we had almost every LUA_PATH wrong.
I found a better way to generate a valid default LUA_PATH, aka patch LUA_ROOT instead of patching all the rests. It's much simpler and robust and revelead our values were quite bad ^^'' .
My last problem is that now that I include ./?.lua in the pattern, the setup hook includes every derivation in LUA_PATH because of

local topDir="${absPattern%%\?*}" 

Once this is fixed we are good to go.

@lolbinarycat unrelated to this PR but mentioning this in case you haven't noticed but the propagatedBuildInputs are not added to LUA_PATH as it should. loadFromPropagatedInputs is never called pkgs/development/interpreters/lua-5/wrap.sh . This will be my next task since it's now hitting me when developing https://github.com/nvim-neorocks/rocks.nvim/

[lolbinarycat]

@teto

My last problem is that now that I include ./?.lua in the pattern, the setup hook includes every derivation in LUA_PATH

you shouldn't need to include ./?.lua in the pattern, since it should be in LUA_PATH_DEFAULT, not in $LUA_PATH, and certainly not in LuaPathSearchPaths. (these should be seperate, LUA_PATH_DEFAULT controls the default value of $LUA_PATH, while LuaPathSearchPaths controls what gets added to $LUA_PATH by the envHook.)

[teto]

yes, this was badly worded, what I meant is now that ./?.lua is part of LUA_PATH_DEFAULT

[lolbinarycat]

1. see my latest issue for why LUA_PATH_DEFAULT should not be used for the envHook
2. as far as i can tell, wrap.sh is not part of the normal envHook, but instead is a separate thing. (it builds a lua interpreter with the given packages embedded into it, its not clear why this would be useful instead of just including those packages as dependencies, maybe it should be deprecated?)

[lolbinarycat]

why are we even patching LUA_PATH_DEFAULT via luaconf.h? to remove the /usr/... prefixes?

luajit explicitly says that patching luaconf.h shouldn't be needed, yet we do it anyways.

[teto]

why are we even patching LUA_PATH_DEFAULT via luaconf.h? to remove the /usr/... prefixes?

Yes and also to have ;; work correctly. Wrapping the interpreter wouldn't work for ;;. I've added a mention in the manual.

luajit explicitly says that patching luaconf.h shouldn't be needed, yet we do it anyways.

I've added LUA_ROOT to makeFlags after finding in the luajit source code an #ifdef LUA_ROOT (contrary to puc lua that just defines it). It works ok but I wondered if it is even necessary, maybe it's just the default behavior.

While fixing luajit and updating the golden test, I also noticed that the LUA_PATH contains share/lua/luajit-2.1/?.lua which doesn't work well with the setup hook. I've quickly introduced LUA_PATH_PATTERN and redirected lua.pkgs.luaLib.LuaPathList to interpreter-specific versions to account for it but I am open to suggestions. For instance we could ditch de the LUA_PATH_PATTERN thing for now and just check for lua files in a derivation to add it to the LUA_PATH.

[teto]

arf and I wanted to update the release notes but seeing you duplicating the work I hurried up and forgot. Quite tired now but interested in your feedback/test. nixpkgs-review is great for that.

[lolbinarycat]

Yes and also to have ;; work correctly.

what do you mean by "correctly"

i'm not advocating for wrapping lua, i'm advocating for setting PREFIX for luajit and LUA_ROOT for lua5

we only need to patch it to find its own system libraries, other modules will be located by the envHook

[lolbinarycat]

the reason i started my own PR is because i wanted something more minimalist, this one makes a lot of changes, often without good reason (such as renaming variables)

[lolbinarycat]

nixpkgs-review is great for that.

i can't run it on my system, both due to the network of doing a full clone of nixpkgs, and due to the memory load of nix-build.

[teto]

thanks for not giving up with this old geezer and the good reviews ! Planning to merge tomorrow after CI passes.

What's next:

1. we might want to test the default LUA_CPATH as well ?
2. actually fix pkgs/development/interpreters/lua-5/hooks/setup-hook.sh to not use luapathsearchpaths but the relative paths from the default LUA_PATH
3. remove luaPathList (it is used in several modules to work around the bugs we had)
4. fix the wrapLuaProgram to actually wrap propagated lua dependencies. Should solve so many bugs.

[lolbinarycat]

thanks for not giving up with this old geezer and the good reviews ! Planning to merge tomorrow after CI passes.

of course! i just want my favorite programming language to have a functioning nixpkgs ecosystem :)

What's next:

1. we might want to test the default LUA_CPATH as well ?

yep, as soon as this gets merged i'm planning on writing a bunch more unit tests, namely making sure LUA_PATH gets set correctly when there are propogatedBuildInputs.

2. actually fix  pkgs/development/interpreters/lua-5/hooks/setup-hook.sh to not use luapathsearchpaths but the relative paths from the default LUA_PATH

this would actually be a regression! these need to be kept separate for the simple reason that they are often different.

in lua 5.2 and below, only ./?.lua is checked in terms of relative paths, but for system modules, both .../?.lua and
.../?/init.lua are checked.

3. remove luaPathList (it is used in several modules to work around the bugs we had)

deduplication is good, although we should make sure we don't break the override interface too much.

4. fix the wrapLuaProgram to actually wrap propagated lua dependencies. Should solve so many bugs.

yep, and add unit tests for that.

[teto]

of course! i just want my favorite programming language to have a functioning nixpkgs ecosystem :)

Looking forward to someone taking over the lua maintenance (after some "running-in" ofc, I hope google-translate did a good job on "running-in") ;) As you could tell I am not an avid lua user, I just maintained the ecosystem because I needed it for neovim.

this would actually be a regression! these need to be kept separate for the simple reason that they are often different.
in lua 5.2 and below, only ./?.lua is checked in terms of relative paths, but for system modules, both .../?.lua and .../?/init.lua are checked.

so maybe not the default LUA_PATH but right now the lua setup hook looks at the luaPathList for patterns, which doesn't contain ./?.lua so derivations with that pattern are not included in the generated LUA_PATH.

My guess (going to sleep sry ^^') is that in:
nix-shell -p lua5_2 lua5_2.somePackageWithALuaFileAtTheRoot
lua wont be able to require the module at /nix/store/HASH-somePackageWithALuaFileAtTheRoot/toto.lua

Feel free to ping me on matrix if you think this helps clear misunderstandings, my nickname is the same.

[lolbinarycat]

Looking forward to someone taking over the lua maintenance (after some "running-in" ofc, I hope google-translate did a good job on "running-in") ;) As you could tell I am not an avid lua user, I just maintained the ecosystem because I needed it for neovim.

i probably will set myself as a lua maintainer after this PR gets merged, although getting access to the community builder would let me actually perform real tests on the lua ecosystem.

My guess (going to sleep sry ^^') is that in:
nix-shell -p lua5_2 lua5_2.somePackageWithALuaFileAtTheRoot
lua wont be able to require the module at /nix/store/HASH-somePackageWithALuaFileAtTheRoot/toto.lua

as far as i know this is intended behavior. derivations (other than source derivations) generally should not put their files in the derivation root. if there are any derivations that do this, those derivations should be changed.

[teto]

so lua starts getting smarter with lua5_4 and doesn't add separators if there is nothing before or after ;;. I've updated the tests to distinguish luajit_2_0 and luajit_2_1. We'll eventually have to remove the leading ; from golden_LUA_PATH when testing with LUA_PATH != ;; but that's ok for now.

[vcunat]

I see that some builds broke because of this PR (succeed when I revert it locally):
libpeas2 https://hydra.nixos.org/build/254049435/nixlog/3/tail
knot-resolver https://hydra.nixos.org/build/254409026/nixlog/2/tail

I haven't investigated why, so far.
/cc staging-next PR #298548

[lolbinarycat]

it seems that buildLuarockPackage puts files in /share/lua/5.1, even withing luajitPackages. luajit doesn't check there by default, nix used to patch it to, but we just removed that behavior.

we need to either reinstitute that, or change where files are installed (the latter feels more in line with how other lua*Packages work, but would at the very least need a changelog entry)

[lolbinarycat]

to clarify my last point, it seems odd that luajitPackages and lua51Packages install to the same dir, but lua52Packages has a seperate dir.

[teto]

nix run .#luajitPackages.luarocks -- config shows luarocks installs it in 5.1 (lua_module_path) even though it recognized a luajit interpreter. I dont know what is the correct behavior here, we should check with hisham or luajit folks. I just found https://stackoverflow.com/questions/6803582/luajit-not-seeing-rocks-installed-by-luarocks with no answer.

[teto]

Looks like luarocks doesn't special case luajit https://github.com/luarocks/luarocks/blob/c16bdbc4cbb98c560fb7e8d4895d4d84afca2eb9/src/luarocks/core/cfg.lua#L194 so unless the system changes the default config lua_modules_path, it will install in 5.1 . I can't find any ticket of someone using luajit+luarocks with this issue that someone must have had ? anyway time to sleep

[teto]

@vcunat so I had a look at both builds. In knot-resolver logs, we see in the logs that LUA_PATH contains paths towards libraries but http. As the derivation was a bit convoluted, I chose to look at libpeas which looked simpler. There doesn't seem to be an issue with the lua hooks.

$ nix develop ~/nixpkgs#libpeas2
$ genericBuild
...
meson.build:404:2: ERROR: Problem encountered: Lua51 requested but failed to locate suitable Lua51 and LGI support
$ echo $LUA_PATH  
`;;;/nix/store/yn9ncrx9fhp9zqpqisqcq601v2wlgp5w-lua5.1-lgi-0.9.2-1/share/lua/5.1/?.lua;/nix/store/yn9ncrx9fhp9zqpqisqcq601v2wlgp5w-lua5.1-lgi-0.9.2-1/share/lua/5.1/?/init.lua` 
$ cd ../  # go to build/ 's parent directory
$ mesonConfigurePhase # try again with same outcoe

so the path looked correct (folders exist) ?
I am not familiar with meson at all. Is it possible that meson's check is faulty ? or at least doesn't cover our usecase ? In the knot's resolver logs we can see /usr/local paths which should not appear so I wonder if there isn't any any magic somewhere in meson or build scripts of those 2 packages.

[vcunat]

Let me provide a trivial test case that regresses:

$ env NIX_PATH=nixpkgs=. nix-shell --pure -Q -p luajitPackages.lua luajitPackages.http
nix-shell$ luajit
> require('http.request')
stdin:1: module 'http.request' not found:
[...]

EDIT: it also broke with lua51Packages and lua52Packages, not just luajit.

[vcunat]

meson itself isn't able to do any lua-specific stuff. Of course, scripts written in meson can invoke lua/luajit, etc.

[vcunat]

In the knot's resolver logs we can see /usr/local paths which should not appear

Feel free to patch luajit not to inject such paths by default. I don't think it's closely related to this regression, though.

[teto]

thanks for the easy repro

seemed like I used bash globstar wrong (I had tested but not correctly apparently, http is a good test because there is no lua file directly in share/lua/5.1/ but only in its http subfolder), aka a **.lua pattern doesn't work, it has to be **/*.lua

If you adapt the for loop in pkgs/development/interpreters/lua-5/hooks/setup-hook.sh with

-  for _file in ${absPattern/\?/\*\*}; do
+ for _file in ${absPattern/\?/\*\*\/\*}; do

it works. Gonna send a patch tomorrow, so dont revert if you can ;)

[vcunat]

I wasn't hasty because... touching the file causes a nontrivial rebuild. This bug is in master now, BTW. (because I don't like delaying security fixes too much)

[teto]

Ty. Opened a fix with its test at #300905 (test fails before the fix). Rebuilding my config with it