Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying specific Lua runtime version #287

Closed
serozhenka opened this issue Feb 7, 2024 · 12 comments
Closed

Add support for specifying specific Lua runtime version #287

serozhenka opened this issue Feb 7, 2024 · 12 comments
Labels
enhancement New feature or request Fund

Comments

@serozhenka
Copy link

serozhenka commented Feb 7, 2024

Is your feature request related to a problem? Please describe.
As stated in Redis documentation, the only supported Lua runtime is Lua 5.1. Logically, I would also expect it to work with Lua 5.x, because the new minor versions should be backward-compatible with previous minor versions. However, that is not the case. In Lua 5.2 unpack was completely removed and replaced with table.unpack.
So when using Lua with fakeredis, the only available version is 5.4 and in the real-world environment it's 5.1. Being unable to fix the Lua version and use one of somebody's choice forces to do such a fix to make it working:

table.unpack = table.unpack or unpack

Describe the solution you'd like
Probably, a good solution will be to pass Lua version with an environment variable like FAKEREDIS_LUA_VERSION and default it to the latest one in case it's not present or lupa doesn't support runtime for this version.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@serozhenka serozhenka added the enhancement New feature or request label Feb 7, 2024
@cunla
Copy link
Owner

cunla commented Feb 7, 2024

Oh, so redis supports only LUA 5.1 while fakeredis supports LUA 5.4.
I think it would be better to default it to 5.1 on fakeredis.

@serozhenka
Copy link
Author

@cunla yeah, this is also doable and valid. I just thought it might break other users' scripts, so it might be better to leave a default as 5.4 for this minor version with an ability to select a custom one, and then make 5.1 a default in the next major version.

@cunla
Copy link
Owner

cunla commented Feb 7, 2024

The issue is with lupa2.0.
If you install lupa 1.14.1 lua 5.1 works fine

@serozhenka
Copy link
Author

serozhenka commented Feb 7, 2024

Sure, but can it be the feature of the fakeredis to have the ability to set a specific Lua version or at least have 5.1 as a default one? I don't think lupa maintainers will ever update 1.14.x version.

@cunla
Copy link
Owner

cunla commented Feb 7, 2024

A bug should be opened on lupa package. They claim 5.1 is supported in v2.

@serozhenka
Copy link
Author

serozhenka commented Feb 7, 2024

Sorry, but I don't get it. How does opening an issue ticket in lupa package fix this problem? And what even is the exact problem with lupa?

As stated in lupa documentation:

The binary wheels include different Lua versions as well as LuaJIT, if supported. By default, import lupa uses the latest Lua version, but you can choose a specific one via import.

At the time of writing, the latest version of Lua is 5.4. So when you do

from lupa import LuaRuntime

it's the 5.4 that gets used. But in this specific Redis scenario, the Lua version is fixed to 5.1, so importing the LuaRuntime should be as follows:

from lupa.lua51 import LuaRuntime

@cunla
Copy link
Owner

cunla commented Feb 7, 2024

So with lupa2 - from lupa.lua51 import LuaRuntime does not exist

@serozhenka
Copy link
Author

ok, my bad for not checking it, I see what you mean. For some strange reason lupa ships only with 5.[234] and not 5.1.

@cunla
Copy link
Owner

cunla commented Mar 23, 2024

I created an issue on lupa, closing this issue.

@cunla cunla closed this as completed Mar 23, 2024
@serozhenka
Copy link
Author

I also opened the issue some time ago right here.
Basically, we'll have it with the next release, so I wouldn't close this issue as you'll still need to import from lupa.lua51 and not lupa

@cunla
Copy link
Owner

cunla commented Mar 23, 2024

Good point...

@cunla cunla reopened this Mar 23, 2024
@cunla cunla closed this as completed Mar 24, 2024
@serozhenka
Copy link
Author

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Fund
Projects
None yet
Development

No branches or pull requests

2 participants