Merge with mlua about rlua HOT 32 CLOSED

Published!

from rlua.

Move complete ✅

from rlua.

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.

from rlua.

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.

from rlua.

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::*;?

from rlua.

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

from rlua.

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 ?

from rlua.

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

from rlua.

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.

from rlua.

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.

from rlua.

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

from rlua.

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.

from rlua.

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

from rlua.

Fixed in v0.9.5

Confirmed, thanks!

from rlua.

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.

from rlua.

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.

from rlua.

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.

from rlua.

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.

from rlua.

Totally fine, the CircleCI should work now

from rlua.

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.

from rlua.

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.

from rlua.

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?

from 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).

from rlua.

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

from rlua.

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.

from 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?

from rlua.

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

from rlua.

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.

from rlua.

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.

from rlua.

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

from rlua.

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

from rlua.

Very happy to see this!

from rlua.