-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: Add option(LazyPath, ...)
support
#21380
Conversation
6272af7
to
8b94c67
Compare
8b94c67
to
5c05097
Compare
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.
Nice, thank you. This is a good improvement, happy to merge this if you try making this requested change.
5c05097
to
e6392b0
Compare
Also adds support for `[]const LazyPath` in a similar vein, and refactors a few other bits of code.
e6392b0
to
d9ad14a
Compare
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 for the adjustments.
} | ||
|
||
return true; | ||
} | ||
|
||
fn userLazyPathsAreTheSame(lhs_lp: LazyPath, rhs_lp: LazyPath) bool { |
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 think this is ok for now but probably a future enhancement will be to make it detect correctly that a cwd_relative path pointing to the same file as a src_path as equal.
Another future enhancement will be to make it a public eql
method instead of this private, strangely named function. I see that you did it because of a nearby similarly named function but the same thing applies to that one.
@@ -2065,22 +2163,17 @@ pub fn dependencyFromBuildZig( | |||
} | |||
|
|||
fn userValuesAreSame(lhs: UserValue, rhs: UserValue) bool { | |||
if (std.meta.activeTag(lhs) != rhs) return false; |
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 don't like this function because it means that the syntax sucks so much people are using std.meta instead of basic syntax for unions. However, you can keep it here as a visible blight to encourage me to make it nicer to access the active tag of a union with zig syntax. I think removing @unionTag
was a mistake. The motivation was since it could be implemented in the standard library, the language could be simplified. But I think that was bad reasoning.
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.
what was the reasoning for ==
and !=
not working on unions directly?
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'd guess ambiguity and consistency, it's not obvious whether the comparison interacts with the payload or not
Also adds support for
[]const LazyPath
in a similar vein, and refactors a few other bits of code.The main purpose of this addition is to enable passing
LazyPath
s from depender to dependency, ie say you generate a directory via someRun
step, and you want to pass that as an argument to your dependency, which will use it as, say a search path for an executable, static resources, or what have you.I've wanted/needed this on a few occasions. It can sort of be hacked in by exposing the functionality via
@import("dep-name")
, but it would be great to have this as a native capability.