Allow relative lua modules at runtime#273342
Conversation
|
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_1I 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): My |
|
@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. |
|
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. 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. |
|
@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 |
|
All good, figured it out! From the base directory of this repo I was able to run the test like this: |
I removed the patch and hopefully simplified the way This also fixed a failing test, tested via |
| golden_LUA_PATH='./share/lua/${lua.luaversion}/?.lua;./?.lua;./?/init.lua' | ||
|
|
||
| assertStringEqual "$generated" "$golden_LUA_PATH" | ||
| assertStringContains "$generated" "$golden_LUA_PATH" |
There was a problem hiding this comment.
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.
|
Do you mind if I merge your code in new PRs ? so we are just missing |
|
I don't mind at all! I just ran your command on CentOS 7 and it looks like you're right. Only |
better error message as well extracted from NixOS#273342 written by heyarne
better error message as well extracted from #273342 written by heyarne
|
i've stumbled across this again, and while it seems like lua is fixed, luajit most certainly is not. |
|
i belive i found the underlying bug that stops this from working, it can affect both luajit and lua5. |
|
@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 . feel free to open an issue if it's not good yet. |
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
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.