Prepare Zig package

[hryx]

Hello! Here is a first swipe at making ziglua available as a package dependency. I'm sure you will catch things I haven't covered yet but thought I would open this and get the discussion rolling.

Overview

This is a breaking change. Currently, the intended way to use ziglua is to @import("path/to/ziglua/build.zig") and call its helpers to create the artifacts. While this should be possible in a dependency context once ziglang/zig#14279 is implemented, this isn't the intended way to define CompileSteps in a remote Zig package (as you know). This PR sets things up for the intended way:

• Add build.zig.zon
• Refactor build.zig to define a module and create artifacts (static or shared lib) directly on the *std.Build
• Install headers along with the library
• Pass target and optimize options to the library CompileStep, as well as the custom version and shared options, all of which can be specified locally or by a depending project's build.zig

I threw in some miscellaneous changes, but open to discussion if you don't agree with them:

• Add version info to Lua shared/static lib
• Define LUA_USE_APICHECK if and only if opitimize=Debug
• Add zig-out to .gitignore (not a relevant change but this is usually desirable)

Deficiencies

There is a limitation in std.Build.Module that makes the new module painful to use in depending projects: ziglang/zig#14719 . There is a workaround in that issue, which I have verified as working in a downstream project. If you approve of the general approach in this PR, we may still want to hold of on merging this until Module.addIncludePath() exists.

With this change as it is currently, it is possible to run zig build test -Dtarget=..., which will create a cross-compiled test executable which cannot be run. That's probably not ideal, but I'm not sure what the correct solution is there. Options that come to mind are disallowing/ignoring -Dtarget in the test step (simple) or providing a custom test runner that tests the library on a foreign target (less simple).

I haven't made any relative documentation changes yet.

[natecraddock]

Thank you for working on this! I'll take a closer look later, but at first glance everything looks really good.

With this change as it is currently, it is possible to run zig build test -Dtarget=..., which will create a cross-compiled test executable which cannot be run. That's probably not ideal, but I'm not sure what the correct solution is there. Options that come to mind are disallowing/ignoring -Dtarget in the test step (simple) or providing a custom test runner that tests the library on a foreign target (less simple).

I think this is a non-issue? Because zig build test compiles and immediately executes the tests, I can't imagine a scenario when one would want to do this on a different target. So I don't think anything needs to be done. Maybe I'm misunderstanding though?

[hryx]

I can't imagine a scenario when one would want to do this on a different target

I agree this isn't something one would want to do, I was (vaguely) concerned that a user has the option to do it at all. But I'm pretty sure I was overthinking it — I'm cool with leaving it as-is since that's not really a valid scenario. :)

[natecraddock]

Alright, I took some time today to look over this and here are my thoughts.

First, I think we should hold off on merging until ziglang/zig#14719 is fixed. I don't think providing a package manager workflow makes sense until it's actually useful! I know the workaround exists, but as you suggested it would probably be better to hold off until there is a better workflow.

Second. I'm still trying to wrap my head around the new build and package systems, so I hope I'm understanding everything correctly. Here's my test build.zig and build.zig.zon files:

const std = @import("std");

pub fn build(b: *std.Build) void {
    const target = b.standardTargetOptions(.{});
    const optimize = b.standardOptimizeOption(.{});

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

    const exe = b.addExecutable(.{
        .name = "test",
        .root_source_file = .{ .path = "src/main.zig" },
        .target = target,
        .optimize = optimize,
    });
    exe.linkLibrary(ziglua.artifact("lua"));
    exe.addModule("ziglua", ziglua.module("ziglua"));

    exe.install();
}

.{
    .name = "test",
    .version = "0.0.0",
    .dependencies = .{
        .ziglua = .{
            .url = "https://github.com/hryx/ziglua/archive/8236cb910bb5e1b38cec3ba4d5694bd649287f91.tar.gz",
            .hash = "1220df5219887e079386d3ed403038afe341bb7b3228953411187ee4e1614ef9a108",
        }
    },
}

From what I understand, this is the correct way to do things. As we know, the module doesn't work with finding the headers, but otherwise it's good.

What I'm unsure on is how to specify the desired version of Lua from here. For the module this actually seems easy. I would refactor your PR build.zig to look like this

    b.addModule(.{
        .name = "lua_51",
        .source_file = .{ .path = "src/ziglua-5.1/lib.zig" },
    });
    b.addModule(.{
        .name = "lua_52",
        .source_file = .{ .path = "src/ziglua-5.2/lib.zig" },
    });
    // ...

Then the dependant package can just do exe.addModule("ziglua", ziglua.module("lua_54"));

But I'm unsure on how the library artifact can be chosen in the same way without compiling all 4 versions. I know b.dependency() accepts arguments, so maybe that's where you could specify it?

But another thought is that

    exe.linkLibrary(ziglua.artifact("lua"));
    exe.addModule("ziglua", ziglua.module("ziglua"));

To me this is an issue. What if someone specifies conflicting versions for the library artifact and the module? It sounds like it might be nice if a module can include an artifact as well. It sounds like someone else has thought about this too: https://discord.com/channels/605571803288698900/785499283368706060/1078577841723166780

But maybe if we can pass the Lua version via b.dependency() then instead of providing different modules we can just have one "ziglua" module and one "lua" artifact and use the ziglua build.zig logic to only build artifact and provide only one module.

Sorry for rambling on. Those are my current thoughts. Thanks again for working on this!

[natecraddock]

Oh look at this! Someone has a PR: ziglang/zig#14731

[hryx]

Oh look at this! Someone has a PR: ziglang/zig#14731

Holy crap, that was quick!

Here's my test build.zig and build.zig.zon files:

Yep, that looks 100% correct. But additionally, a user can pass any build options that ziglua defines to the dependency — this should answer your question about version.

myproject/build.zig:

    const ziglua = b.dependency("ziglua", .{
        .target = target,
        .optimize = optimize,
        .version = .lua_52,
        .shared = true,
    });

This works because the argument to b.dependency() is untyped, you can pass anything; ziglua will receive the values at build time from b.option("version") etc. It inherits optimize and target from b.standardTarget|OptimizeOptions().

If the author of "myproject" wanted to add -Dversion=... and -Dshared to their zig build CLI flags instead of using hardcoded values like above, they instead could do const shared = b.option(...) and then pass those values to b.dependency(...). (This will be slightly nicer in the future when the user can reference your LuaVersion enum via ziglang/zig#14279, but it still works with enum literals.)

The takeaway here is that when ziglua is used as a dependency, its calls to b.option() no longer get values directly from the command line, they must be provided by the outer project.

Anyway, just to clear up version specifically. As before, version is a build option in this dependency, and with this PR, both the module and artifact are based on that version. So in a single build, it's not possible to generate a module and lib that have conflicting versions. The result is that the user specifies the version in the dependency, and they only need to access the module or artifact as "ziglua" and "lua".

It would be possible to define a unique module/lib for each Lua version separately, but my gut says the PR's approach is more ergonomic, and practical since most projects will want to stick with a single API version at a time.

Let me know if that all makes sense and if I misunderstood any of your points!

[natecraddock]

But additionally, a user can pass any build options that ziglua defines to the dependency

🤦‍♂️ Oh, I tried that but totally spelled .version = .lua_52, incorrectly. Glad to know that works as intended.

Anyway, just to clear up version specifically. As before, version is a build option in this dependency, and with this PR, both the module and artifact are based on that version. So in a single build, it's not possible to generate a module and lib that have conflicting versions. The result is that the user specifies the version in the dependency, and they only need to access the module or artifact as "ziglua" and "lua".

Knowing that the dependency stuff works, I totally agree. One module and one lua artifact that depend on the version passed to the dependency is the way to go!

So I think this PR is good. And when package manager supports more CompileStep apis on modules we can update this PR and then merge!

[jiacai2050]

Hi, I wonder if build.zig.zon is required for making ziglua as package?

AFAIK, the only requirement for a package is to provide module or compilation artifact in build.zig.
Take https://github.com/andrewrk/libz for example, only build.zig is there.

[BanchouBoo]

It's not required, but it doesn't hurt to have.

[hryx]

Yep, it's not required. But there are a couple things that a build.zig.zon uniquely provides:

• Package name, description, and version info, which are not currently possible to declare with the Build API directly.
• The ability for the package manager to gather information and build a dependency graph without compiling and executing all the dependencies' build.zig scripts.

I don't think the package manager takes advantage of either of these yet, but probably will in the future.

Additionally, a downstream project needs a build.zig.zon to depend on another package.

[natecraddock]

Yeah I see no harm in including a build.zig.zon in this PR. If it turns out it's unused, we can just delete it. But it sounds like it will eventually be useful so might as well include it now since it's such a small file.

[kassane]

Samples: How to use zig-pkg manager (v0.11/master)
https://github.com/InKryption/zig-pm-example-lib
https://github.com/InKryption/zig-pm-example-app

[efjimm]

Is the header issue the only thing blocking this?

A fix would be to install the header files without the 'lua' subdirectory, as the top level 'include' directory gets added to the include path of anything that links the resulting artifact. You could just change the cIncludes in the zig bindings to include the lua subdirectory e.g. @cInclude("lua.h") to @cInclude("lua/lua.h")

I tried this out locally and could use the package just fine by adding the module + linking the lua artifact.

[natecraddock]

@efjimm Thanks for looking into this!

Is the header issue the only thing blocking this?

To my knowledge, a Zig module cannot expose an artifact or a header, but there is an open PR discussed above that will add this.

But it sounds like you got something working? I'm not following exactly what you did, but could you please share more? If there is an existing solution I would like to know. Thanks!

[efjimm]

Artifacts are exposed separately from modules - calling Build.installArtifact exposes that artifact in the package. The problem was that addIncludePath does not work when using ziglua as a dependency. This meant that when compiling ziglua as a dependency it wouldn't be able to find the headers that ziglua calls @cInclude on. However, the lib.installHeader function actually adds ziglua's zig-out/include directory to the include path of anything that links lib. So ziglua can find the headers by referring to them relative to that directory - e.g. changing @cInclude("lua.h") to @cInclude("lua/lua.h").

[natecraddock]

@efjimm sorry it took a few days to get around to this, but thanks so much! I hadn't realized the dependency.artifact() function was available, and your suggestion for how to fix the header problem works wonderfully. I merged in your repo.

Thanks also to @hryx for the work on starting this PR many months ago! Happy to see this finally finished.