Skip to content

std.Build.LazyPath: upgrade API usages of source-relative path#19623

Merged
andrewrk merged 2 commits intomasterfrom
LazyPath
Apr 12, 2024
Merged

std.Build.LazyPath: upgrade API usages of source-relative path#19623
andrewrk merged 2 commits intomasterfrom
LazyPath

Conversation

@andrewrk
Copy link
Member

The first commit deletes the deprecated API and then makes all the upgrades needed to pass the standalone tests.

The second commit reverts the API breakage. The second commit can be reverted once 0.12.0 is tagged.

@castholm re:

'path' lazy paths are relative to the build root of some step, inferred from the step
in which they are used. This means that we can't dupe such paths, because they may
come from dependencies with their own build roots and duping the paths as is might
cause the build script to search for the file relative to the wrong root.
As a temporary workaround, we convert build root-relative paths to absolute paths.
If/when the build-root relative paths are updated to encode which build root they are
relative to, this workaround should be removed.

The *Build argument to LazyPath.dupe was only being used for its allocator, and all instances share the same allocator. So converting a path prematurely to an absolute path was unnecessary and introduced unwanted absolute paths.

-    /// Duplicates the file source for a given builder.
+    /// Copies the internal strings.
+    ///
+    /// The `b` parameter is only used for its allocator. All *Build instances
+    /// share the same allocator.
     pub fn dupe(self: LazyPath, b: *Build) LazyPath {

The previous commit deleted the deprecated API and then made all the
follow-up changes; this commit reverts only the breaking API changes.
This commit can be reverted once 0.12.0 is tagged.
@castholm
Copy link
Contributor

castholm commented Apr 11, 2024

@andrewrk

So converting a path prematurely to an absolute path was unnecessary and introduced unwanted absolute paths.

It wasn't unnecessary and was required specifically for installLibraryHeaders when the library came from a dependency. Prior to eee5400, the following repro failed:

// foo/build.zig
const std = @import("std");

pub fn build(b: *std.Build) void {
    const libfoo = b.addStaticLibrary(.{
        .name = "foo",
        .target = b.resolveTargetQuery(.{}),
        .optimize = .Debug,
    });
    libfoo.addCSourceFile(.{ .file = b.addWriteFiles().add("empty.c", "") });
    libfoo.installHeader(.{ .path = "header.h" }, "foo/foo.h"); // <------------ Note: .path, not .src_path
    b.installArtifact(libfoo);
}
// foo/header.h
#define FOO_FOO "FOO"
// build.zig
const std = @import("std");

pub fn build(b: *std.Build) void {
    const exe = b.addExecutable(.{
        .name = "main",
        .target = b.resolveTargetQuery(.{}),
        .link_libc = true,
    });
    exe.addCSourceFile(.{ .file = b.addWriteFiles().add("main.c",
        \\#include <stdio.h>
        \\#include <foo/foo.h>
        \\int main(void) {
        \\    printf(FOO_FOO "\n");
        \\    return 0;
        \\}
    ) });

    const foo_dep = b.dependency("foo", .{});
    const libfoo = foo_dep.artifact("foo");

    exe.linkLibrary(libfoo);
    exe.installLibraryHeaders(libfoo);
    b.getInstallStep().dependOn(&b.addInstallArtifact(exe, .{
        .h_dir = .{ .override = .header }, // <---------------- This is the thing that fails
    }).step);

    const run_exe = b.addRunArtifact(exe);
    run_exe.step.dependOn(b.getInstallStep());

    const run = b.step("run", "Run the app");
    run.dependOn(&run_exe.step);
}
// build.zig.zon
.{
    .name = "main",
    .version = "0.0.0",
    .dependencies = .{
        .foo = .{
            .path = "foo",
        },
    },
    .paths = .{""},
}
zig build run
error: unable to update file from 'C:\temp\header.h' to 'C:\temp\zig-out\include\foo\foo.h': FileNotFound

Note that 0b7123f made installing headers to the .header install dir the default for .lib artifacts, so until the deprecated .path is fully removed, they will run into this problem if they use installLibraryHeaders with lazy paths specified with .path.

It's probably not worth keeping the workaround; the fix is just to use the new b.path(...), but for a few days the workaround was justified 🙂

pub fn path(b: *Build, sub_path: []const u8) LazyPath {
assert(!fs.path.isAbsolute(sub_path));
if (fs.path.isAbsolute(sub_path)) {
std.debug.panic("sub_path is expected to be relative to the build root, but was this absolute path: '{s}'. It is best avoid absolute paths, but if you must, it is supported by LazyPath.cwd_relative", .{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to*

@andrewrk
Copy link
Member Author

Understood, thank you for the clarification!

@andrewrk andrewrk merged commit 10ff81c into master Apr 12, 2024
@andrewrk andrewrk deleted the LazyPath branch April 12, 2024 06:35
scheibo added a commit to pkmn/engine that referenced this pull request Apr 13, 2024
yamashitax added a commit to yamashitax/zig-curl-fork that referenced this pull request Apr 13, 2024
yamashitax added a commit to yamashitax/zig-curl-fork that referenced this pull request Apr 13, 2024
yamashitax added a commit to yamashitax/zig-curl-fork that referenced this pull request Apr 13, 2024
jiacai2050 pushed a commit to jiacai2050/zig-curl that referenced this pull request Apr 14, 2024
)

Ref: ziglang/zig#19623

The changes here are trivial, only a small change required to be up to
date with master.

Tested with `0.12.0-dev.3639+9cfac4718`
raysan5 pushed a commit to raysan5/raylib that referenced this pull request May 29, 2024
* Fix for issue #4010

Split the code for Zig's master branch and >= 0.12.0 due to changes in ziglang/zig#19623

* Restore the cache_include path which was removed in error

Accidently removed a couple lines I didn't mean to 🙈
CorrectRoadH pushed a commit to CorrectRoadH/ncdu that referenced this pull request Mar 26, 2025
 * LazyPath now stores `Build` owner inside in
 ziglang/zig#19623 and
 ziglang/zig#19597 .

Signed-off-by: Eric Joldasov <bratishkaerik@landless-city.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants