lua: fix longstanding bug in lua envHook causing relative module imports to stop working

[lolbinarycat]

Description of changes

originally thought to only affect luajit, this bug can also affect lua5, such as when running via
nix-shell -p lua luaPackages.toml.

this bug was caused by the lua envHook overriding the default package path whenever it found a module.

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.

[lolbinarycat]

Fixes #113110

[lolbinarycat]

see extended commit message for more info

[lolbinarycat]

Superceeds #273342

[teto]

Could you add a test of what you want in pkgs/development/interpreters/lua-5/tests please ?

#!/usr/bin/env nix-shell
#! nix-shell -i sh -p lua luaPackages.toml
lua -e 'print(package.path)'
echo "lua path: $LUA_PATH"
source ./assert.sh
# here adds some tests (lua path contains XX) and exit 1 when fails

output:

./share/lua/5.2/?.lua;./?.lua;./?/init.lua

NB: this will eventually need to target staging considering the number of rebuilds

[lolbinarycat]

@teto

Could you add a test of what you want in pkgs/development/interpreters/lua-5/tests please ?

#!/usr/bin/env nix-shell
#! nix-shell -i sh -p lua luaPackages.toml
lua -e 'print(package.path)'
echo "lua path: $LUA_PATH"
source ./assert.sh
# here adds some tests (lua path contains XX) and exit 1 when fails

the existing test for luajit basically already does this (it was broken before, and now it works), if i was to add more tests it would be to test the actual inclusion of modules instead of just checking the path.

i don't mind writing some more unit tests for lua, as it could clearly use them, but i would want to do that as a separate PR as the scope would be larger than just this 1 thing that is already tested for.

NB: this will eventually need to target staging considering the number of rebuilds

do i need to do something about that? i'm fairly new to nixpkgs.

[lolbinarycat]

also, my intent with this PR was to keep it as minimal as possible so that it can easily be reviewed. this is 4 year old bug that affects most multi-file lua projects, so my main priority was to just get it fixed.

[teto]

it fixes nix-build -A luajit.tests so that's cool but
nix-shell -p lua luaPackages.argparse still ignores local files

$ nix-shell -p lua luaPackages.argparse
$ echo $LUA_PATH
/nix/store/p8rnf25wx45kggkm7c2dcxk6svsn0hbh-lua5.2-argparse-0.7.1-1/share/lua/5.2/?.lua;/nix/store/p8rnf25wx45kggkm7c2dcxk6svsn0hbh-lua5.2-argparse-0.7.1-1/share/lua/5.2/?/init.lua

[lolbinarycat]

it fixes nix-build -A luajit.tests so that's cool but nix-shell -p lua luaPackages.argparse still ignores local files

$ nix-shell -p lua luaPackages.argparse
$ echo $LUA_PATH
/nix/store/p8rnf25wx45kggkm7c2dcxk6svsn0hbh-lua5.2-argparse-0.7.1-1/share/lua/5.2/?.lua;/nix/store/p8rnf25wx45kggkm7c2dcxk6svsn0hbh-lua5.2-argparse-0.7.1-1/share/lua/5.2/?/init.lua

@teto nix-shell -p always uses the system channel.

run NIX_PATH=nixpkgs=$PWD/default.nix nix-shell -p luaPackages.argparse lua to use the local branch of nixpkgs.

[teto]

Indeed sry ! when the number of rebuilds > 2000, we usually target the "staging" branch, which becomes available in nixos-unstable 1/2 weeks later. Can you target staging (convert the PR first to draft avoids pinging dozens of people when making a mistake so I recommand that) https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging and then I will merge.

[lolbinarycat]

@teto rebased!

[teto]

ty

[infinisil]

Fyi, I believe the pkgs/by-name check failed due to the base branch change, something relating to #282707 or so (not 100% sure). Shouldn't occur again in the future, but please ping me if you see the check failing. It generally shouldn't be ignored, I'm continuously improving it.

[vcunat]

I now verified that this particular merge broke some lua* stuff (just reverting it fixes the builds), but I haven't investigated deeper. Examples:

• linuxPackages.bcc

  /nix/store/8xaaf8ya90arb768ghd2ln0ad41zys5n-luajit-2.1.1693350652/bin/luajit: unknown luaJIT command or jit.* modules not installed
  

• knot-resolver

  tests/meson.build:27:4: ERROR: Problem encountered: missing luajit package: cqueues
  

• luaPackages.sqlite and various vimPlugins.* affected by
  https://hydra.nixos.org/build/251939883
  but there's probably also some other issue in that case, as just reverting this PR doesn't fix it
• awesome
• libpeas2

/cc the corresponding staging-next PR #292500

[teto]

@vcunat these errors look related to an invalid luarocks config. I've submitted some changes in that area so I'll have a look.

EDIT still it's weird, I usually run in my fork whatever I submitted to staging and have no issue..

[vcunat]

@teto: perhaps we should revert this PR for now, so that there's not so much time pressure on this? The staging-next PR is basically waiting for this now, delaying other changes including security fixes.

[teto]

I had a very quick look and yeah I could not find a link with my luarocks-related changes (might still be the case though, but I use these in my fork and it works well). If you are sure this is the responsible PR, let's revert it and put it back after more rigourous testing later.

lolbinarycat has prompted us to look into lua issues and we will do so in a more orderly manner #286822 . I have some CPU capacity to run nixpkgs-review on the PRs but we will have to slow down binary cat :)

anyway thanks for asking/letting us know about the status, maintaining staging-next is hard work and we'll try to be more careful on the next merge.

[lolbinarycat]

nixpkgs-review probably would have caught this, unfortunately it's too much for my laptop, as is trying to test things on staging, as that requires rebuilding all of stdenv, and that deadlocks everything due to running out of memory

[vcunat]

That sounds done wrong (well, maybe the tool could be blamed). Rebuilding together with staging changes. Either way, typical laptops aren't suitable for such things; it's good to have a remote more powerful machine at hand.

[lolbinarycat]

@vcuncat yeah, that would be nice, but unless someone feels like giving me access to their personal build server, i think i'm fairly stuck with what i've got.