ziglua

ziglua copied to clipboard

The luajit and lua51 internal API didn't take a mode variable, meaning the function signature was different between these two versions and lua 5.2+. Add an unused Mode to the loadFile51 signature to allow building with luajit/lua51

The checkVersion function calls luaL_checkversion, which was only introduced in lua 5.2. Add an empty call when using a version less than 5.2

Hey thank you for this contribution! So I'm actually unsure how I want to address this. From the perspective of making the API consistent across all Lua versions this is a good fix. But I'm not sure if that is the direction I want to take ziglua.

For example, you fixed loadFile and checkVersion, but there are many other functions with inconsistencies between the Lua versions.

Currently the approach to handle this is to use ziglua.lang in your code to handle any cases where a function differs or isn't available. Would this be a reasonable approach for you? I'm happy to discuss this further to find other solutions

On Sat Apr 20, 2024 at 6:35 PM AKDT, Nathan Craddock wrote:

Hey thank you for this contribution! So I'm actually unsure how I want to address this. From the perspective of making the API consistent across all Lua versions this is a good fix. But I'm not sure if that is the direction I want to take ziglua.

Completely understand!

Currently the approach to handle this is to use ziglua.lang in your code to handle any cases where a function differs or isn't available. Would this be a reasonable approach for you? I'm happy to discuss this further to find other solutions

I'm not sure if this addresses my issue. From a basic use case, i want to use luajit and call "doFile". To do this, I think the most simple reproducer is:

const std = @import("std");
const ziglua = @import("ziglua");

const Lua = ziglua.Lua;

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    var lua = try Lua.init(&allocator);
    defer lua.deinit();

    try lua.doFile("foo.lua");
}

This doesn't work in lua51 or luajit because doFile passes two parameters to lua.loadFile. Is the intention that I should do something like:

const std = @import("std");
const ziglua = @import("ziglua");

const Lua = ziglua.Lua;

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    var lua = try Lua.init(&allocator);
    defer lua.deinit();

    switch (ziglua.lang) {
        .luajit, .lua51 => {
	    try lua.loadFile("foo.lua");
	    try lua.protectedCall(0, ziglua.mult_return, 0);
        }, 
        else => try lua.doFile("foo.lua"),
    }
}

I can certainly do this in my code since I know which version I am using and can avoid calling doFile directly, which let's me bypass the switch and leaves me with a net +1 LOC - no big deal for me.

I think where this is confusing is that lua 5.1 does have a dofile method which takes a single param, but this api is not currently accessible in ziglua. I fully understand if that is your design choice, I'd just petition that you reconsider making this API available for lua51/luajit :).

Reference: https://www.lua.org/manual/5.1/manual.html#pdf-dofile

-- Tim

I see it wasn't quite clear the trace I was calling to get to why I fixed loadFile. I am calling dofile("foo.lua") in luajit, which in turn calls loadFile with two parameters. Perhaps the fix for this should actually be a switch in the doFile function for how it calls loadFile:

    /// Loads and runs the given file
    /// See https://www.lua.org/manual/5.4/manual.html#luaL_dofile
    pub fn doFile(lua: *Lua, file_name: [:0]const u8) !void {
        // translate-c failure
	switch (lang) {
	    .luajit, .lua51 => try lua.loadFile(file_name),
	    else => try lua.loadFile(file_name, .binary_text),
	}
        try lua.protectedCall(0, mult_return, 0);
    }

-- Tim

I guess email replies don't work well with code formatting.

I think the second fix keeps the ziglua 5.1 api the same as lua5.1 (one param for loadFile) and also allows lua5.1 users to call doFile since it only passes one param at the callsite - I believe this addresses your concern but let me know if I am missing something!

Hey there! So sorry for leaving this hanging so long. I've been super swamped the last month. I'll take a closer look later today / over the next few days and get back to you

I see it wasn't quite clear the trace I was calling to get to why I fixed loadFile. I am calling dofile("foo.lua") in luajit, which in turn calls loadFile with two parameters. Perhaps the fix for this should actually be a switch in the doFile function for how it calls loadFile:

/// Loads and runs the given file
/// See https://www.lua.org/manual/5.4/manual.html#luaL_dofile
pub fn doFile(lua: *Lua, file_name: [:0]const u8) !void {
    // translate-c failure
    switch (lang) {
        .luajit, .lua51 => try lua.loadFile(file_name),
        else => try lua.loadFile(file_name, .binary_text),
    }
    try lua.protectedCall(0, mult_return, 0);
}

It took me reading this PR several times, and the source code to comlink to finally figure it out what you explained so simply... sometimes reading is hard 😂

I just merged a fix with this above suggested approach. Thanks! The reason this slipped by is I never added test coverage for loadFile and doFile 🙈 That is also part of the commit.

Thanks for raising this issue, and sorry for the delay!

Also I'm leaving checkVersion untouched, but feel free to open an issue or PR if you think there is something to be done there.