Closed khvzak closed 8 months ago
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
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 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:
pub type Context<'lua> = &'lua Lua
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)to_lua
with into_lua
and use mlua::IntoLua as ToLua
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;
RluaExt
adding the dummy context
(perhaps marked as deprecated)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.
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
.
https://github.com/mlua-rs/rlua/pull/297
There are some remaining issues:
&[u8]
or similar to &str
, which to me seems convenient but not necessarily correct as Lua strings are not required to be utf-8.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 Luaf64::MAX
intof32
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: https://github.com/mlua-rs/rlua/commit/e7461874cbe6c6cd4abf952c645dc6b6ee1c37ee
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 (https://github.com/rust-num/num-traits/pull/186) seems fine to me to keep the new behavior.
Ok, I've updated the branch; I've fixed a couple of tests and disabled/removed the ones that are now less relevant. I've also added a README. Unfortunately the CircleCI tests are no longer running as the repo has moved - I'm not sure how easy it is to hook those up again.
I've requested CircleCI access to the mlua-rs
organisation in the hope that means the CI can be run before a release. @khvzak if you don't think that's appropriate maybe we can figure something else out.
Totally fine, the CircleCI should work now
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.
It does, apart from the failures, thanks! Is there an equivalent to
rlua
'slua-no-oslib
feature? (It stops the Luaos
library from being compiled at all; apparently this is needed for iOS). Hmm, unless that's no longer needed ifLUA_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:
Must use luaL_newstate() for 64 bit target
messages, which seems suspicouslimit_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 suspicousThis 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 toluaL_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 inStdLib
) 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!
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:
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.