-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat(builtin): npm_package_bin can produce directory output #1164
Conversation
@@ -8,26 +8,45 @@ _ATTRS = { | |||
"outs": attr.output_list(), | |||
"args": attr.string_list(mandatory = True), | |||
"data": attr.label_list(allow_files = True, aspects = [module_mappings_aspect]), | |||
"out_dir": attr.string(), |
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.
should be added to doc string for rule and comment on $@
usage in args
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.
could out_dir be a label to give the user more freedom as to where it will be? as a string it is limited to subdirs of the package
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 think that's possible since https://docs.bazel.build/versions/master/skylark/lib/actions.html#declare_directory is the only API to produce TreeArtifact and it takes string - and you can't use a label to reference something that doesn't exist yet
anyway it's probably good that the output directory has to be inside the package that owns it?
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.
👍
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.
added missing doc
Also handle the case of args=["--out-dir=$@"]
(it's a substring of the arg)
thanks @c-parsons for advising on this |
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.
LGTM
may also include targets that produce or reference npm packages which are needed by the tool | ||
outs: identical to [genrule.outs](https://docs.bazel.build/versions/master/be/general.html#genrule.outs) | ||
outs: similar to [genrule.outs](https://docs.bazel.build/versions/master/be/general.html#genrule.outs) | ||
out_dir: use this instead of `outs` if you want the output to be a directory |
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.
nit: mention this is a string and relative to the current package?
No description provided.