Merge with mlua

@kyren

Hello,

I'm mlua maintainer, the lua bindings library you probably already know about.
I started it ~5 years ago to simply add module module support to rlua, but then mlua evolved into a full featured independent library with their own set of features, maintaining the pre-context rlua API style.

Since beginning of mlua, the rlua has changed hands and became part of amethyst. Unfortunately the library slowed down its development a little and is left behind mlua.
This is just a short list of new features and improvements supported by mlua: more Lua versions (including Luau); async functions, module mode support, serialization, new userdata api, webassembly support, and many more.
Apart from that, mlua is tuned for high performance (see benchmarks), about twice faster than rlua and proved itself in high loaded production environments for a long time.

I think it's unfortunate that now community is split between the two libraries and it would be more beneficial for everyone and the Rust comunuty to have a single high quality library. For example the tealr crate supports both libraries under feature flags that I believe is not very convenient.

That's why I'm reaching out to the rlua maintainers with proposal to merge both libraries into a single one. In particular, I'm proposing to:

Move rlua from amethyst into a new (mlua) organization. Unfortunately rlua has no code and/or features that would be useful in mlua, so probably would be easier to freeze current repo and move development and user enquiries/support into the mlua one.
Release a new rlua version as a mirror for mlua codebase and at least for some time continue mirroring.

I spoke with @kyren about this recently and she is happy about the merger and would transfer rlua if she still owned it.

Please, let me know what you think about it. It's completely fine if you have another views on rlua would prefer to continue development independently.

@erlend-sh

Hi,

I'm generally positive about your proposal. I agree that mlua has more momentum (and users, according to the crates.io stats) and features. I'm not likely to make more than glacial progress.

I will have a look at converting one of my projects from rlua to mlua to get a feel for how easy or not it is to do. From the sound of it that shouldn't be difficult.

Maybe there are some details to work out, all being well.

Move rlua from amethyst into a new (mlua) organization. Unfortunately rlua has no code and/or features that would be useful in mlua, so probably would be easier to freeze current repo and move development and user enquiries/support into the mlua one.

If development stops then I don't think it's vital to move it (and I think you have the same maintainer rights as I do, looking back at #174 ). I guess it would make it clearer that mlua would be the replacement. I'm not a github owner so I don't know who would have the rights to move this repo if we decide on that though - maybe @erlend-sh ?

Release a new rlua version as a mirror for mlua codebase and at least for some time continue mirroring.

By "mirror" do you mean a wrapper with a dependency on mlua and a lib.rs containing something like pub use mlua::*;?

I will have a look at converting one of my projects from rlua to mlua to get a feel for how easy or not it is to do.

Take a look to v0.9 release notes as this version has some changes, including renaming ToLua trait / etc.

If development stops then I don't think it's vital to move it

I just feel that amethyst org is not the right place for rlua. I don't insist on this, however. The org I mentioned is mlua-rs where I move mlua too.

By "mirror" do you mean a wrapper with a dependency on mlua and a lib.rs containing something like pub use mlua::*;?

Yeah, seems the easiest option would be re-export mlua symbols and make an announcement about the changes.

Generally mlua almost reached v1.0 point, what I work on is creating a book (similar to rhai) about all aspects of usage.

Anyway, if we reached consensus and would like to proceed, then I need permissions on crates.io

@jugglerchris

Seems fine 👍

Anyway, if we reached consensus and would like to proceed, then I need permissions on crates.io

Do you have the necessary owner privileges give those permissions @jugglerchris ?

@erlend-sh

@erlend-sh Yes, I'm owner on crates.io, but not on the github project.

@khvzak I converted my project. The main things I needed to do:

I added (in a shim crate) a pub type Context<'lua> = &'lua Lua
And added a fn context(&self, f: F) -> R { f(&self) } (I actually added to a wrapper type I already had, but I guess it could be an extension RluaCompat trait or something)
Added some FromLua impls for userdata
Replaced to_lua with into_lua and use mlua::IntoLua as ToLua
A few small changes to error handling, and a unit test which relied on a particular error message string.

It was mostly painless with the context hack, so it seems like there can be a migration path that's not too hard.

Added some FromLua impls for userdata

There is helper #[derive(mlua::FromLua)] to automate this process.
Also maybe you don't really need to clone userdata, as can use UserDataRef<T> or UserDataRefMut<T> helpers.

Move complete ✅

There is helper #[derive(mlua::FromLua)] to automate this process.

This macro didn't work with my type, which was:

struct LuaPtr<T:?Sized> { ... }

It returned:

error[E0658]: associated type bounds are unstable
   --> crates/rlua-extras/src/lib.rs:132:19
    |
132 | pub struct LuaPtr<T:?Sized> {
    |                   ^^^^^^^^
    |
    = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
    = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable

Looks like the macro needs to leave out the bounds in some parts of the expansion.

I'm thinking there should be a 0.20 release of rlua with:

pub use mlua::*
type Context<'lua> = &'lua Lua;
An extension trait RluaExt adding the dummy context (perhaps marked as deprecated)
A README.md explaining about the change and pointing to mlua for new projects.

How does that sound to everyone? It could be done as a separate rlua-transitional repo (since it would be mostly empty), or just as a "delete most things" change on the now-moved rlua repo.

I can put a proposed version of this, probably at the weekend.

@jugglerchris

pub use mlua::*

Would be nice to get doc re-exported too (not sure if it's possible).

All other points makes sence, thanks @jugglerchris !

just as a "delete most things" change on the now-moved rlua repo.

I would upvote for "delete". The current version can be moved to 0.19 branch for fast access.

This macro didn't work with my type, which was:

struct LuaPtr<T:?Sized> { ... }

It returned:

error[E0658]: associated type bounds are unstable
   --> crates/rlua-extras/src/lib.rs:132:19
    |
132 | pub struct LuaPtr<T:?Sized> {
    |                   ^^^^^^^^
    |
    = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
    = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable

Looks like the macro needs to leave out the bounds in some parts of the expansion.

Fixed in v0.9.5

Fixed in v0.9.5

Confirmed, thanks!

I'm working through making the rlua tests pass with the mlua wrapper. There are a bunch of small differences that didn't come up in my other project - at least this will help make a more comprehensive migration guide.

I've pushed a branch with my attempt to make the rlua tests pass with only minor changes when based on mlua.

#297

There are some remaining issues:

I haven't gone through the compile-fail tests (they're mostly failing, but might just be specific errors that need updating)
Some APIs have changed from &[u8] or similar to &str, which to me seems convenient but not necessarily correct as Lua strings are not required to be utf-8.
The test for panics not being caught by Lua pcall fails (test_error). Maybe an mlua bug?
rlua by default doesn't allow loading Lua bytecode (as this is not considered safe); mlua doesn't seem to do this.
test_num_conversion is failing - converting a Lua f64::MAX into f32 is not returning an error as expected.

Some APIs have changed from &[u8] or similar to &str, which to me seems convenient but not necessarily correct as Lua strings are not required to be utf-8.

It's mostly about (userdata) method/function names and usually in direction from Rust to Lua. Eg. it makes a little sense in UserDataMethods::add_method to accept arbitrary bytes as you would not be able to call that method in Lua code without black magic to retrieve metatable (which itself require debug module) and looking in it.

The other thing I remember is named registry values takes &str too.

The test for panics not being caught by Lua pcall fails (test_error). Maybe an mlua bug?

It's actually documented behaviour. By default mlua does not patch Lua runtime (including pcall) but for this particular case you can create Lua instance with an option LuaOptions::catch_rust_panics set to false to enable this behaviour. I'm not sure it's even used by anyone.
Generally the way rlua does patching is not async/coroutine friendly as Lua is unable to cross C call boundary in patched (x)pcall version which prevents yielding. Normally in Lua pcall is implemented using continuation functions that works well with coroutines.
mlua automatically resume panics when they cross over to Rust side and is usually enough.

rlua by default doesn't allow loading Lua bytecode (as this is not considered safe); mlua doesn't seem to do this.

Yeah, this is a deliberate decision. It's usually needed only in sandbox mode and in this case is up to a user to disable any unwanted functionality they want (eg. fs access, loading code, executing programs, etc).
There is a way to crash/segfault your program in 100% safe Rust eg. by writing to /proc/self/mem file but it does not mean this functionality should be disabled.

Generally I try to not patch Lua runtime and for example in module mode, it's just not acceptable. The only exception is loading binary modules but it's not a runtime patching rather removing a C loader (keeping all functionality genue).

test_num_conversion is failing - converting a Lua f64::MAX into f32 is not returning an error as expected.

You should see f32::INFINITY according mlua tests. Seems it was a behaviour change in num-traits >= 0.2.13 .
I see the rlua commit about it: e746187
I just accepted the new behaviour actually in the next major mlua release (it was v0.5.0).

I don't have strong opinion on this, but the num-traits motivation about this (rust-num/num-traits#186) seems fine to me to keep the new behavior.

It does, apart from the failures, thanks! Is there an equivalent to rlua's lua-no-oslib feature? (It stops the Lua os library from being compiled at all; apparently this is needed for iOS). Hmm, unless that's no longer needed if LUA_USE_IOS is set appropriately.

Luau does not has os lib, Lua 5.4 detect ios (target) compilation in build script to set LUA_USE_IOS automatically.

Thanks.
I'm close to CI passing again, except for luajit:

I notice I get a lot of Must use luaL_newstate() for 64 bit target messages, which seems suspicous
The limit_execution_instructions seems not to finish (or I wasn't patient enough) - maybe something's not working with the hooks?

I notice I get a lot of Must use luaL_newstate() for 64 bit target messages, which seems suspicous

This is from system luajit which is very likely too old. mlua support vendored (builtin) luajit (latest rolling version, updated a day ago) which has no issues with 64bit targets.
Anyway, mlua has extra checks and fallback to luaL_newstate automatically (which is happening and the reason why you getting this message).

The limit_execution_instructions seems not to finish (or I wasn't patient enough) - maybe something's not working with the hooks?

Could you try adding these lines.

I suspect the limit_execution_instructions worked in rlua because JIT module was never loaded (it does not present in StdLib) so jit compilation was never enabled in rlua.

I notice I get a lot of Must use luaL_newstate() for 64 bit target messages, which seems suspicous

This is from system luajit which is very likely too old. mlua support vendored (builtin) luajit (latest rolling version, updated a day ago) which has no issues with 64bit targets. Anyway, mlua has extra checks and fallback to luaL_newstate automatically (which is happening and the reason why you getting this message).

I'm using the LuaJIT ~2.1.0, from the package in Ubuntu 22.04.

https://luajit.org/install.html says "Make sure you use luaL_newstate. Avoid using lua_newstate, since this uses the (slower) default memory allocator from your system (no support for this on 64 bit architectures)." Has that changed in the rolling version?

The limit_execution_instructions seems not to finish (or I wasn't patient enough) - maybe something's not working with the hooks?

Could you try adding these lines.

That (or something slightly modified) does seem to do the trick, thanks.

I suspect the limit_execution_instructions worked in rlua because JIT module was never loaded (it does not present in StdLib) so jit compilation was never enabled in rlua.

Interesting - I think you're right.

https://luajit.org/install.html says "Make sure you use luaL_newstate. Avoid using lua_newstate, since this uses the (slower) default memory allocator from your system (no support for this on 64 bit architectures)." Has that changed in the rolling version?

Providing a default memory allocator definitely works for newer luajit on 64 bit targets, but older version returns null. Most dists uses ~7 years old luajit. I checked the code, it requires allocations in a right address range (lower 47 bits).

Using a rust (global) allocator actually has more advantages, for example you can use jemalloc/mimalloc for entire rust app including lua allocations. I doubt that the luajit allocator is faster than jemalloc/mimalloc.
In any case for mlua it's important to control memory allocations as many optimization tricks depend on this plus sandboxing/memory limitation requires custom allocator.

I've merged #297 . Does anyone see any problem with my doing a cargo publish?

I've merged #297 . Does anyone see any problem with my doing a cargo publish?

Looks good to me. 🚢
I'll prepare a post (for reddit).

Published!

Very happy to see this!

Navigation Menu

Repository navigation

Merge with mlua #294

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Merge with mlua #294

Description

Activity

jugglerchris commented on Jan 21, 2024

khvzak commented on Jan 21, 2024

erlend-sh commented on Jan 22, 2024

jugglerchris commented on Jan 22, 2024

khvzak commented on Jan 22, 2024

erlend-sh commented on Jan 23, 2024

jugglerchris commented on Jan 23, 2024

jugglerchris commented on Jan 23, 2024

jugglerchris commented on Jan 23, 2024

khvzak commented on Jan 24, 2024

khvzak commented on Jan 25, 2024

jugglerchris commented on Jan 28, 2024

jugglerchris commented on Jan 28, 2024

jugglerchris commented on Jan 28, 2024

khvzak commented on Jan 28, 2024

4 remaining items

khvzak commented on Feb 4, 2024

jugglerchris commented on Feb 4, 2024

khvzak commented on Feb 4, 2024

khvzak commented on Feb 4, 2024

khvzak commented on Feb 4, 2024

jugglerchris commented on Feb 5, 2024

jugglerchris commented on Feb 5, 2024

jugglerchris commented on Feb 5, 2024

khvzak commented on Feb 5, 2024

jugglerchris commented on Feb 6, 2024

khvzak commented on Feb 7, 2024

jugglerchris commented on Feb 7, 2024

makspll commented on Mar 19, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions