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

Support system and vendored Lua libraries #75

Open
terrablue opened this issue Mar 24, 2024 · 7 comments
Open

Support system and vendored Lua libraries #75

terrablue opened this issue Mar 24, 2024 · 7 comments
Labels
feature New feature or request

Comments

@terrablue
Copy link

terrablue commented Mar 24, 2024

Hey, I'm using your ziglua project to devendor lua in my redis fork. However, the vendored lua exposes two additional functions that it seems upstream doesn't otherwise expose (https://github.com/terrablue/zkv/blob/master/deps/lua/src/lua.h#L361-L362).

Would you mind exposing those functions as well? I find it very convenient to use ziglua as a dependency instead of rolling out my own.

@natecraddock
Copy link
Owner

natecraddock commented Mar 25, 2024

Hey! I took a look at how redis/zkv uses Lua, and it seems that they have made several modifications. For example, the readonly attribute of tables doesn't exist in standard Lua 5.1. They also link against some libraries like lua_cjson. So this would require adding more than just the function declarations to this repo.

Even if this was only adding the following

LUA_API void lua_enablereadonlytable (lua_State *L, int index, int enabled);
LUA_API int lua_isreadonlytable (lua_State *L, int index);

I would still be hesitant to add it. Because Lua is so small and easy to modify, I'm sure Redis isn't alone in making modifications to Lua, and I don't want to set a precedent in modifying the Lua sources bundled with Ziglua. Hopefully that is understandable.

But! I do think there is a solution here. I think Ziglua should be able to support system and vendored Lua sources. Currently it only supports using the Zig package manager Lua in the build.zig.zon.

I know you want to remove your vendored Lua in zkv, but I'm really not sure you can do that because of the modifications. (You could move it to a separate repo though and use the Zig package manager to pull it in at build time.) What do you think about that?

@natecraddock
Copy link
Owner

Also I think I'll move this issue to the Ziglua repo because I think it makes more sense there

@terrablue
Copy link
Author

terrablue commented Mar 25, 2024

Hey, thanks. So far I've got this: https://github.com/terrablue/zua, which is working.

But if possible, it would be nice to deduplicate work, and Ziglua is already doing some nice binding work. Seeing as I generally want to transition zkv to using Zig not just for the build but also for the source code, Ziglua is attractive here.

Perhaps it could be possible to use a repository as a target in addition to the targets you have in build.zig.zon? That would probably allow me to get rid my own build.zig.zon/build.zig in the zua repository.

(Edit: also, feel free to move the issue, I do think it better fits now in the Ziglua repo.)

@natecraddock
Copy link
Owner

I agree, it would be ideal if Ziglua can integrate nicely with other Lua sources. I'll take a look later today on possible solutions and follow up.

I'll move the issue

@natecraddock natecraddock transferred this issue from natecraddock/lua Mar 25, 2024
@natecraddock natecraddock changed the title call to undeclared function 'lua_enablereadonlytable'; ISO C99 and later do not support implicit function declarations Support system and vendored Lua libraries Mar 26, 2024
@natecraddock natecraddock added the feature New feature or request label Mar 26, 2024
@natecraddock
Copy link
Owner

I just solved the first part of this in 3b85c5d. This ensures only the required version of Lua is fetched from the package manager.

Next we need to add a way to link against a different version of Lua. Because this use case is also needed for linking against system Lua libraries, I renamed this issue to reflect both needed features.

Here are my ideas on how to proceed:

  1. You add zua as a dependency in your build.zig.zon
  2. You add ziglua as a dependency in your build.zig.zon
  3. When doing b.dependency("ziglua", .{}) you pass in an option to use a custom build of Lua. This bypasses using the package manager and building Lua inside of ziglua
  4. You link your custom Lua (zua) to the project
  5. Hopefully it all works 🤞

So more concretely, you might do something like this

const zua = b.dependency("zua", .{
    .target = target,
    .optimize = optimize,
});
const ziglua = b.dependency("ziglua", .{
    .target = target,
    .optimize = optimize,

    // Zua (the Lua used by Redis) is Lua 5.1
    .lang = .lua51,

    // Link a custom build of Lua
    .source = .custom,
});

// Now link zua and ziglua to your project

I think we can find a way to get this working, though I haven't tested. I'm open to other ideas though if you have them!

One issue is Ziglua itself can't bind any custom functions in the ziglua.Lua struct. But we can expose ziglua.c so you could call ziglua.c.lua_enablereadonlytable() for example.

Looking at ziglang/zig#18778, this seems very similar to the use case of system packages. I wonder if there should be some support in the build system / package manager for this.

@terrablue
Copy link
Author

Sounds good to me! It there anything in particular I'd need to do for the zua repository (which I'd probably then rename to lua-51-zkv or so) beyond what I'm currently doing, that is building it using build.zig/build.zig.zon?

@natecraddock
Copy link
Owner

At the moment, I don't think there is anything you need to do with zua. Again, this is just my first thoughts on how to proceed. I don't know for sure if it will work, but I'll give it a go sometime in the next few days and let you know how it goes

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

No branches or pull requests

2 participants