-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
build/LazyPath: Add dirname #18371
build/LazyPath: Add dirname #18371
Conversation
bc17f98
to
d043266
Compare
Ah, I assumed "zig-cache" as the basename in that test. |
Adds a variant to the LazyPath union representing a parent directory of a generated path. ```zig const LazyPath = union(enum) { generated_dirname: struct { generated: *const GeneratedFile, up: usize, }, // ... } ``` These can be constructed with the new method: ```zig pub fn dirname(self: LazyPath) LazyPath ``` For the cases where the LazyPath is already known (`.path`, `.cwd_relative`, and `dependency`) this is evaluated right away. For dirnames of generated files and their dirnames, this is evaluated at getPath time. dirname calls can be chained, but for safety, they are not allowed to escape outside a root defined for each case: - path: This is relative to the build root, so dirname can't escape outside the build root. - generated: Can't escape the zig-cache. - cwd_relative: This can be a relative or absolute path. If relative, can't escape the current directory, and if absolute, can't go beyond root (/). - dependency: Can't escape the dependency's root directory. Testing: I've included a standalone case for many of the happy cases. I couldn't find an easy way to test the negatives, though, because tests cannot yet expect panics.
The test will fail if run like this: ``` zig build test --cache-dir zig-local-cache ``` The test should use the basename provided by b.cache_root instead.
Build was green but there was a small conflict following #18160 which had to be resolved. |
Definitly a better solution than #16803. Thanks for taking care of that topic! |
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.
Thanks, nice work.
In the future I want to improve the implementation by moving away from absolute file paths. I took the time to write up that issue here: #18450
However, in the meantime, this is an improvement, and that issue can be a follow-up change. Thanks to your new tests, it will be easy to make sure that the feature continues to work at that time.
Adds a variant to the LazyPath union representing a parent directory
of a generated path.
These can be constructed with the new method:
For the cases where the LazyPath is already known
(
.path
,.cwd_relative
, anddependency
)this is evaluated right away.
For dirnames of generated files and their dirnames,
this is evaluated at getPath time.
dirname calls can be chained, but for safety,
they are not allowed to escape outside a root
defined for each case:
so dirname can't escape outside the build root.
If relative, can't escape the current directory,
and if absolute, can't go beyond root (/).
Testing:
I've included a standalone case for many of the happy cases.
I couldn't find an easy way to test the negatives, though,
because tests cannot yet expect panics.
Resolves #17411