Allow relative lua modules at runtime #273342

rrrnld · 2023-12-10T13:30:28Z

Description of changes

I have made some changes to allow for relative lua package imports. This addresses the following two issues:

I decided to set these directly on the definitions of the lua / luajit derivations, so it's still possible to build versions of these packages that follow the previous, stricter path logic. I still think it makes sense to set these by default so that lua scripts are more easily portable (which in my mind is a nod to the fact that it's quite common to see lua scripts distributed without any package manager, but just as a tarball of a bunch of files).

Things done

Add a 👍 reaction to pull requests you find important.

rrrnld · 2023-12-10T14:24:00Z

It looks like some of the issues were already addressed:

# 2873a731 == nixpkgs-unstable at the time of writing

echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#lua
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#lua5_1
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#lua5_2
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#lua5_3
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#lua5_4
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#luajit
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#luajit_2_0
# ./share/lua/5.2/?.lua;./?.lua;./?/init.lua
echo "print(package.path)" | nix run github:NixOS/nixpkgs/2873a731#luajit_2_1

I encountered the issue like this (fennel is a lisp-like that compiles to lua and it's distributed as a lua package; running it you'd assume to be able to load lua packages like you normally would):

# nix run github:NixOS/nixpkgs/2873a731#lua.pkgs.fennel -- -e '(. package :path)'
# formatted for clarity:
/nix/store/psx2jaqhvnizbn6sraapdc8afl2d77xv-fennel-luarocks-config.lua/share/lua/5.2/?.lua
/nix/store/psx2jaqhvnizbn6sraapdc8afl2d77xv-fennel-luarocks-config.lua/share/lua/5.2/?/init.lua
/nix/store/nb3wjqhykfd6f3pds9smxdr1xfc4pwaw-luarocks-3.9.1/share/lua/5.2/?.lua
/nix/store/nb3wjqhykfd6f3pds9smxdr1xfc4pwaw-luarocks-3.9.1/share/lua/5.2/?/init.lua
/nix/store/3dgc655m12q6x1prf8jp1386vwgwgv2c-lua5.2-fennel-1.3.1-1/share/lua/5.2/?.lua
/nix/store/3dgc655m12q6x1prf8jp1386vwgwgv2c-lua5.2-fennel-1.3.1-1/share/lua/5.2/?/init.lua
/home/arne/.luarocks/share/lua/5.2/?.lua
/home/arne/.luarocks/share/lua/5.2/?/init.lua

My LUA_PATH and LUA_CPATH are empty, so this is the default behavior. This is the result of the above command after my patch:

/nix/store/a5a7d7l9ckqw63h80zhjpx8i671gappv-fennel-luarocks-config.lua/share/lua/5.2/?.lua
/nix/store/a5a7d7l9ckqw63h80zhjpx8i671gappv-fennel-luarocks-config.lua/share/lua/5.2/?/init.lua
/nix/store/d97swj4m7mhq64wmix69bk5vn9kjdakl-luarocks-3.9.1/share/lua/5.2/?.lua
/nix/store/d97swj4m7mhq64wmix69bk5vn9kjdakl-luarocks-3.9.1/share/lua/5.2/?/init.lua
./?.lua
./?/init.lua
/nix/store/6m9qq5k0sn0sm5fixy1lbbc0hik3lxl7-lua5.2-fennel-1.3.1-1/share/lua/5.2/?.lua
/nix/store/6m9qq5k0sn0sm5fixy1lbbc0hik3lxl7-lua5.2-fennel-1.3.1-1/share/lua/5.2/?/init.lua
/home/arne/.luarocks/share/lua/5.2/?.lua
/home/arne/.luarocks/share/lua/5.2/?/init.lua

rrrnld · 2023-12-19T09:31:19Z

@teto i saw you in a couple of lua issues. can you provide some feedback over whether this is useful at all, or is this misguided? see #273342 (comment) for a closer description of what's happening.

teto · 2023-12-28T14:17:12Z

sry for the delay. First of all thanks for trying to address the issue ! Happy to see folks trying to improve the lua ecosystem.

What I would like to see is a test in pkgs/development/interpreters/lua-5/tests/default.nix to check for the behavior you want (even as a separate PR).

I dont know how involved you want to be but to limit the issues mentioned at #189689 I think it would be great to have luajit be expressed as an overrideAttrs of the default lua interpreter maybe ? that would limit the drift between the 2 and might help with solving the issue.
pkgs/development/interpreters/lua-5/interpreter.nix
pkgs/development/interpreters/luajit/default.nix

At one point I decided to patch the lua interpreter to change the default LUA_PATH. Patching upstream software might not have been the correct call and might have created your issue. Not sure if I want to rollback this yet, or make it configurable.

rrrnld · 2023-12-29T08:02:14Z

@teto thanks for your feedback! i added some tests locally. can you help me figure out how to run them so i don't have to do the git-push-wait-for-ci-to-fail-git-commit-ammend-force-push-dance?

diff --git a/pkgs/development/interpreters/lua-5/tests/assert.sh b/pkgs/development/interpreters/lua-5/tests/assert.sh
index fe5582a0b062..328c4ee7f93a 100644
--- a/pkgs/development/interpreters/lua-5/tests/assert.sh
+++ b/pkgs/development/interpreters/lua-5/tests/assert.sh
@@ -14,3 +14,9 @@ function assertStringEqual {
         fail "Strings differ"
     fi
 }
+
+function assertStringContains {
+    if ! echo "$1" | grep -q "$2" ; then
+        fail "String does not match"
+    fi
+}
diff --git a/pkgs/development/interpreters/lua-5/tests/default.nix b/pkgs/development/interpreters/lua-5/tests/default.nix
index 38479af5f207..02b9e124f0d1 100644
--- a/pkgs/development/interpreters/lua-5/tests/default.nix
+++ b/pkgs/development/interpreters/lua-5/tests/default.nix
@@ -27,6 +27,10 @@ let
         wrapLuaPrograms
       '';
     });
+
+    luaWithModule = lua.withPackages(ps: [
+      ps.lua-cjson
+    ]);
 in
   pkgs.recurseIntoAttrs ({
 
@@ -40,11 +44,22 @@ in
       '';
   };
 
-  checkWrapping = pkgs.runCommandLocal "test-${lua.name}" ({
+  checkWrapping = pkgs.runCommandLocal "test-${lua.name}-wrapping" ({
     }) (''
       grep -- 'LUA_PATH=' ${wrappedHello}/bin/hello
       touch $out
     '');
 
+  checkRelativeImports = pkgs.runCommandLocal "test-${lua.name}-import" ({
+    }) (''
+      lua_vanilla_package_path=$(${lua}/bin/lua -e "print(package.path)")
+      lua_with_module_package_path=$(${luaWithModule}/bin/lua -e "print(package.path)")
+
+      assertStringContains "$lua_vanilla_package_path" "./?.lua"
+      assertStringContains "$lua_vanilla_package_path" "./?/init.lua"
+
+      assertStringContains "$lua_with_module_package_path" "./?.lua"
+      assertStringContains "$lua_with_module_package_path" "./?/init.lua"
+    '');
 })

I'm afraid I'm not deep enough into either lua or nix packaging to unify the lua / luajit packages as you described

rrrnld · 2023-12-29T14:34:34Z

All good, figured it out! From the base directory of this repo I was able to run the test like this:

nix build .#lua.tests.checkRelativeImports

rrrnld · 2023-12-30T07:50:19Z

At one point I decided to patch the lua interpreter to change the default LUA_PATH. Patching upstream software might not have been the correct call and might have created your issue. Not sure if I want to rollback this yet, or make it configurable.

I removed the patch and hopefully simplified the way LUA_PATH / LUA_CPATH is configured in the process. This means the path can be configured by overriding LuaPathSearchPaths / LuaCPathSearchPaths if I didn't do anything silly.

This also fixed a failing test, tested via nix-build -A pkgs.lua.passthru.tests.

rrrnld · 2023-12-30T08:17:43Z

pkgs/development/interpreters/lua-5/tests/default.nix

@@ -36,15 +40,30 @@ in
      generated=$(lua -e 'print(package.path)')
      golden_LUA_PATH='./share/lua/${lua.luaversion}/?.lua;./?.lua;./?/init.lua'

-      assertStringEqual "$generated" "$golden_LUA_PATH"
+      assertStringContains "$generated" "$golden_LUA_PATH"


this was necessary because of this:

$ nix log /nix/store/irzzqd4j56jn4j6n91r7qd7jmknbgnwj-test-luajit-2.1.1693350652-check-aliases.drv 1c1 < ./share/lua/5.1/?.lua;./?.lua;./?/init.lua;/nix/store/n99wdp44qbkzkgs4q5c7nkgqzfcfdl6c-luajit-2.1.1693350652/share/lua/5.1/?.lua;/nix/store/n99wdp44qbkzkgs4q5c7nkgqzfcfdl6c-luajit-2.1.1693350652/share/lua/5.1/?/init.lua --- > ./share/lua/5.1/?.lua;./?.lua;./?/init.lua expected "./share/lua/5.1/?.lua;./?.lua;./?/init.lua;/nix/store/n99wdp44qbkzkgs4q5c7nkgqzfcfdl6c-luajit-2.1.1693350652/share/lua/5.1/?.lua;/nix/store/n99wdp44qbkzkgs4q5c7nkgqzfcfdl6c-luajit-2.1.1693350652/share/lua/5.1/?/init.lua" to be equal to "./share/lua/5.1/?.lua;./?.lua;./?/init.lua"

this surfaced after 68dcfd5, before which the same lua 5.2 interpreter was used for all tests.

i'm not sure if this i an intentional difference between lua / luajit.

teto · 2024-01-21T23:14:19Z

Do you mind if I merge your code in new PRs ?
I would like to merge your changes to tests first, and then fix up things differently.
If I run from ubuntu

lua -e "print(package.path)"  
./?.lua;/usr/local/share/lua/5.1/?.lua;/usr/local/share/lua/5.1/?/init.lua;/usr/local/lib/lua/5.1/?.lua;/usr/local/lib/lua/5.1/?/init.lua;/usr/share/lua/5.1/?.lua;/usr/share/lua/5.1/?/init.lua

so we are just missing ?.lua from default LUA_PATH.

rrrnld · 2024-01-22T08:09:55Z

I don't mind at all! I just ran your command on CentOS 7 and it looks like you're right. Only ./?.lua is missing by default,

better error message as well extracted from NixOS#273342 written by heyarne

better error message as well extracted from #273342 written by heyarne

lolbinarycat · 2024-02-15T19:12:20Z

i've stumbled across this again, and while it seems like lua is fixed, luajit most certainly is not.

lolbinarycat · 2024-02-15T22:24:16Z

i belive i found the underlying bug that stops this from working, it can affect both luajit and lua5.

teto · 2024-04-05T18:10:21Z

@heyarne sry it took so long but this should be fine on master. Thanks to your contributions and @lolbinarycat we now have some more tests

nixpkgs/pkgs/development/interpreters/lua-5/tests/default.nix

Line 45 in c67ab4e

    
           "2.0" = ";./?.lua;${lua}/share/luajit-2.0/?.lua;/usr/local/share/lua/5.1/?.lua;/usr/local/share/lua/5.1/?/init.lua;${lua}/share/lua/5.1/?.lua;${lua}/share/lua/5.1/?/init.lua;";

. feel free to open an issue if it's not good yet.

Allow relative lua modules at runtime

1ede414

github-actions bot added the 6.topic: lua label Dec 10, 2023

ofborg bot requested review from sergei-mironov, vcunat, lblasc and thoughtpolice December 10, 2023 17:15

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 2501-5000 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Dec 10, 2023

Add tests

e0b3420

Handle LUA_PATH / LUA_CPATH in one place only

aba22df

arne added 2 commits December 30, 2023 09:06

Fix lua version used in tests

68dcfd5

Fix failing test for luajit

4b8bef6

rrrnld commented Dec 30, 2023

View reviewed changes

teto mentioned this pull request Feb 6, 2024

lua.tests: add test for relative import #286817

Merged

13 tasks

teto added a commit to teto/nixpkgs that referenced this pull request Feb 6, 2024

lua.tests: add test for relative import

c1cbbfc

better error message as well extracted from NixOS#273342 written by heyarne

teto added a commit that referenced this pull request Feb 6, 2024

lua.tests: add test for relative import

ff5b2d7

better error message as well extracted from #273342 written by heyarne

lolbinarycat mentioned this pull request Feb 15, 2024

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

Merged

13 tasks

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024

teto closed this Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow relative lua modules at runtime #273342

Allow relative lua modules at runtime #273342

rrrnld commented Dec 10, 2023 •

edited

Loading

rrrnld commented Dec 10, 2023 •

edited

Loading

rrrnld commented Dec 19, 2023

teto commented Dec 28, 2023 •

edited

Loading

rrrnld commented Dec 29, 2023

rrrnld commented Dec 29, 2023 •

edited

Loading

rrrnld commented Dec 30, 2023 •

edited

Loading

rrrnld Dec 30, 2023 •

edited

Loading

teto commented Jan 21, 2024

rrrnld commented Jan 22, 2024

lolbinarycat commented Feb 15, 2024

lolbinarycat commented Feb 15, 2024

teto commented Apr 5, 2024

Allow relative lua modules at runtime #273342

Allow relative lua modules at runtime #273342

Conversation

rrrnld commented Dec 10, 2023 • edited Loading

Description of changes

Things done

rrrnld commented Dec 10, 2023 • edited Loading

rrrnld commented Dec 19, 2023

teto commented Dec 28, 2023 • edited Loading

rrrnld commented Dec 29, 2023

rrrnld commented Dec 29, 2023 • edited Loading

rrrnld commented Dec 30, 2023 • edited Loading

rrrnld Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

teto commented Jan 21, 2024

rrrnld commented Jan 22, 2024

lolbinarycat commented Feb 15, 2024

lolbinarycat commented Feb 15, 2024

teto commented Apr 5, 2024

rrrnld commented Dec 10, 2023 •

edited

Loading

rrrnld commented Dec 10, 2023 •

edited

Loading

teto commented Dec 28, 2023 •

edited

Loading

rrrnld commented Dec 29, 2023 •

edited

Loading

rrrnld commented Dec 30, 2023 •

edited

Loading

rrrnld Dec 30, 2023 •

edited

Loading