zig icon indicating copy to clipboard operation
zig copied to clipboard

std.Build.Module.linkSystemLibrary() breaks target inheritance

Open ifreund opened this issue 1 year ago • 2 comments
trafficstars

Zig Version

0.12.0 (nothing looks to have changed on master)

Steps to Reproduce and Observed Behavior

I have a zig-wlroots package with idiomatic Zig bindings to the wlroots C library. This package exposes a wlroots Zig module that internally uses @cImport() to obtain the wlroots version defined in the installed C headers:

https://codeberg.org/ifreund/zig-wlroots/src/commit/938a314162ee5ec9abc6dc8e6e930d5e52c87a8b/src/version.zig

This means that the wlroots Zig module must be provided with the include path for the wlroots C headers, which in turn should be obtained using pkg-config. Currently, the only pkg-config integeration in std.Build is the linkSystemLibrary() function.

The problem is that calling std.Build.Module.linkSystemLibrary() on the wlroots module errors due to this line:

https://github.com/ziglang/zig/blob/9be8a9000faead40b1aec4877506ff10b066659c/lib/std/Build/Module.zig#L431

which requires the target to be explicitly specified rather than inherited.

My current workaround is to set the Module.resolved_target field directly before calling Module.linkSystemLibrary():

https://codeberg.org/river/river/src/commit/7fdba05b8249b10d10a2c64c1175429539c01af1/build.zig#L148-L149

Expected Behavior

There are many different changes that could be made which would support this specific use-case well. I'm not sure which option makes the most sense. Here are some reasonable options I can think of:

  1. Make modules inherit include paths from their parent compilation. This would side-step the issue entirely and I would only need a single linkSystemLibrary() call for the parent compilation.

  2. Add a new std.Build API to use pkg-config to only add the required include paths, not link the library. Don't synchronously require a resolved target in this new Module.addIncludePathPkgConfig() function.

  3. Change Module.linkSystemLibrary() to not synchronously require a resolved target. It currently uses this to determine if the requested library name is actually part of libc/libc++. This check could however be deferred until std.Build.Complie.make() if I'm not mistaken.

I don't have a strong opinion on which of these approaches is the best.

ifreund avatar May 22 '24 21:05 ifreund

Would this solve it?

    const wlroots = b.dependency("zig-wlroots", .{
        .target = target,
        .optimize = optimize
    }).module("wlroots");

Along with in zig-wlroots:

    const target = b.standardTargetOptions(.{});
    const optimize = b.standardOptimizeOption(.{});

    _ = b.addModule("wlroots", .{
        .root_source_file = .{ .path = "src/wlroots.zig" },
        .target = target,
        .optimize = optimize,
    });

pfgithub avatar May 23 '24 03:05 pfgithub

@pfgithub setting the target and optimize of the module from standard{Target,Optimize}Options() in zig-wlroots seems to have the same effect regardless of whether or not I pass those values in the dependency() call in the consumer's build.zig. I don't understand what the intended behavior is here.

On a higher level, I think it is indicative of a design flaw if inheritance of the target/optimize options works fine for all cases as long as one doesn't call linkSystemLibrary(). Calling that function doesn't change the fact that I semantically want inheritance and it shouldn't require me to change anything else in my build.zig IMO.

Either inheritance should become explicit through always requiring the user to use standard{Target,Optimize} Options() or it should work regardless of what std.Build API is used.

ifreund avatar May 23 '24 08:05 ifreund