-
Notifications
You must be signed in to change notification settings - Fork 450
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
fix bazel build #189
fix bazel build #189
Conversation
@davidzchen This looks OK to me? What do you think? |
jsonnet = "//:jsonnet", | ||
std = "//:std", | ||
jsonnet = "//cmd:jsonnet", | ||
std = "//stdlib", |
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.
Come to think of it, I think we can remove the std
attribute since the stdlib is baked into the jsonnet binary. @sparkprime is this correct?
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.
You may be right that it's unnecessary, but the jsonnet_to_json_test rule defined in the bazel repo has a default value for std, which is simply wrong for these targets.
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 David's right but we can fix it some other time
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.
Agreed. I will remove the std
attribute from the Bazel Jsonnet rules in a separate change.
Just have a few comments, otherwise LGTM. |
Comments addressed. PTAL. |
LGTM thanks |
* Introduce a Parens AST
Fixes #188