Skip to content

Allow relative lua modules at runtime#273342

Closed
rrrnld wants to merge 5 commits intoNixOS:masterfrom
rrrnld:fix-lua-paths
Closed

Allow relative lua modules at runtime#273342
rrrnld wants to merge 5 commits intoNixOS:masterfrom
rrrnld:fix-lua-paths

Conversation

@rrrnld
Copy link
Contributor

@rrrnld rrrnld commented Dec 10, 2023

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

  • 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:
  • 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.

@github-actions github-actions bot added the 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. label Dec 10, 2023
@rrrnld
Copy link
Contributor Author

rrrnld commented Dec 10, 2023

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

@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 10, 2023
@rrrnld
Copy link
Contributor Author

rrrnld commented Dec 19, 2023

@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
Copy link
Member

teto commented Dec 28, 2023

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
Copy link
Contributor Author

rrrnld commented Dec 29, 2023

@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
Copy link
Contributor Author

rrrnld commented Dec 29, 2023

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
Copy link
Contributor Author

rrrnld commented Dec 30, 2023

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.

golden_LUA_PATH='./share/lua/${lua.luaversion}/?.lua;./?.lua;./?/init.lua'

assertStringEqual "$generated" "$golden_LUA_PATH"
assertStringContains "$generated" "$golden_LUA_PATH"
Copy link
Contributor Author

@rrrnld rrrnld Dec 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

teto commented Jan 21, 2024

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
Copy link
Contributor Author

rrrnld commented Jan 22, 2024

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,

@teto teto mentioned this pull request Feb 6, 2024
13 tasks
teto added a commit to teto/nixpkgs that referenced this pull request Feb 6, 2024
better error message as well
extracted from NixOS#273342

written by heyarne
teto added a commit that referenced this pull request Feb 6, 2024
better error message as well
extracted from #273342

written by heyarne
@lolbinarycat
Copy link
Contributor

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

@lolbinarycat
Copy link
Contributor

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

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

teto commented Apr 5, 2024

@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

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

@teto teto closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants