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.

Sign up for GitHub

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

Adds LazyPath.dirname for getting the containing folder of a LazyPath #16803

Closed
wants to merge 1 commit into from
Closed

Adds LazyPath.dirname for getting the containing folder of a LazyPath #16803

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions lib/std/Build.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1928,6 +1928,51 @@ pub const LazyPath = union(enum) {
.generated => |gen| .{ .generated = gen },
};
}

/// Returns a `LazyPath` that points to the directory name of the current `LazyPath`.
pub fn dirname(path: LazyPath) LazyPath {
const ComputeStep = struct {
step: Step,
input: LazyPath,
output: GeneratedFile,

fn make(comp_step: *Step, progress: *std.Progress.Node) !void {
_ = progress;

const self = @fieldParentPtr(@This(), "step", comp_step);

const realpath = self.input.getPath2(comp_step.owner, comp_step);

self.output.path = resolve(realpath);
}

fn resolve(realpath: []const u8) []const u8 {
return std.fs.path.dirname(realpath) orelse if (std.fs.path.isAbsolute(realpath)) "/" else ".";
Copy link
Member

Choose a reason for hiding this comment

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

On WIndows, we will blindly return / as root for an abs path which is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On WIndows, we will blindly return / as root for an abs path which is wrong.

Actually not quite sure, windows also allows current-drive absolute paths that don't use a drive specifier (\Windows\system32\calc.exe) is valid if your CWD is in C:\. I gotta verify that, will test when i have a windows machine at hand

Copy link
Member

Choose a reason for hiding this comment

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

Either way, whatever the actual drive or path notation is, we should return that instead of /.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it for a while, i think the correct solution here is one of:

  1. Panic and let the user fix it
  2. Pass in a builder, resolve the paths to absolute paths and then invoke dirname on those (using . is also a bad idea here)
  3. Rewrite the Build to allow using Build.relPath(…), Build.cwdPath(…) which both resolve to the correct path internally. This is probably the correct solution here, as .path is currently broken-by-design:
    What will happen if a dependency Build will pass a .{ .path = "local" } to a "higher" builder? It will suddenly point to a file in the local folder instead of the one where the dependency was fetched to.

cc @andrewrk

}
};

switch (path) {
.cwd_relative => |value| return LazyPath{ .cwd_relative = ComputeStep.resolve(value) },
.path => |value| return LazyPath{ .path = ComputeStep.resolve(value) },

.generated => |ptr| {
const child = ptr.step.owner.allocator.create(ComputeStep) catch @panic("oom");
child.* = ComputeStep{
.input = path,
.output = .{ .step = &child.step },
.step = Step.init(.{
.id = .custom,
.name = "dirname",
.owner = ptr.step.owner,
.makeFn = ComputeStep.make,
.first_ret_addr = @returnAddress(),
}),
};
path.addStepDependencies(&child.step);
return LazyPath{ .generated = &child.output };
},
}
}
};

/// In this function the stderr mutex has already been locked.
Expand Down