-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Adds LazyPath.dirname for getting the containing folder of a LazyPath #16803
Conversation
… file or directory.
409fb64
to
b8bbc20
Compare
Nice! This will simplify my |
} | ||
|
||
fn resolve(realpath: []const u8) []const u8 { | ||
return std.fs.path.dirname(realpath) orelse if (std.fs.path.isAbsolute(realpath)) "/" else "."; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 /
.
There was a problem hiding this comment.
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:
- Panic and let the user fix it
- Pass in a builder, resolve the paths to absolute paths and then invoke dirname on those (using
.
is also a bad idea here) - 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but a little bit more work is required to better support Windows :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with supporting the use case (see #17411) however, I think this is not the best way to go about implementing it. I added a diff to that issue to show my counter-proposal for how to implement it.
One more thing that I want to do here is protect against this dirname
function being used to go outside of a root dir. For example, it shouldn't be possible to go from a generated file inside the zig-cache to outside the zig-cache, or from a project-relative file to outside the project. It's less of a security concern issue, and more just that I want to prevent people from putting a bunch of brittle, janky logic in their build scripts.
Superseeded by #18371 |
Use cases: