-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add map_to_output rule #217
base: main
Are you sure you want to change the base?
Conversation
2154464
to
919ce4f
Compare
FYI @alexeagle |
@c-parsons I thought there was a discussion recently about how some of the rules added to skylib already were likely a mistake, and to say on charter the though was less rules and just building block, common rules should live some place else? If that is correct, should the README.md/etc. get updated with a more clear charter and pointer to the proper location for generic rules? |
Indeed, I think we should generally apply some scrutiny to whether a new rule belongs in skylib. For this particular PR, could we get some context as to who we expect to use this? |
Sure, this is for rules_nodejs, they have a need to copy a source file to bazel-bin at the same path, (eg. However, in order to have as less dependency as possible, rules_nodejs will probably just copy this rule to their own repo instead of depending on skylib. So I'm also not very sure if we should add this to skylib repo or not. @alexeagle may have more idea. |
More generally this provides a third option for running binaries/tests:
We like option 3 since it fits our existing ecosystem. In node you generally make a complete dist/ folder and run e.g. a test runner by pointing it to dist/ I would like for users to get it from skylib if we can make the dependency story work (federation?) |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# --- begin runfiles.bash initialization --- |
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.
why not use the shorter v2 init code?
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
"""A rule that copies a source file to bazel-bin at the same execroot path. |
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.
feels like a maintenance burden to have so much doc duplication between the rule declaration and this public re-export - but I guess you're following conventions in the repo?
|
||
map_to_output( | ||
name = "a", | ||
src = "foo/bar/a.txt", |
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 it should be a list of srcs
as it will be common to have more than one file to copy
_map_to_output = "map_to_output", | ||
) | ||
|
||
map_to_output = _map_to_output |
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.
bikeshedding the name: should it be more analogous to the existing rules in this repo? maybe it should start with copy
like copy_file
?
I'm not quite convinced on adding a (canonical) third option for runfiles handling. If this is easier for rules_nodejs, that's one thing -- but it's another to suggest it so strongly as to add the solution to bazel-skylib. As @thomasvl alluded to, we're aiming to take a stricter stance on things that belong in skylib. We'd like to err on the side of saying no to brand new features. Any thoughts, @laurentlb ? |
Nothing node specific here, I think we just have more ambitions to run existing test runners under Bazel rather than rewrite everything like java_test et al |
I'd suggest not adding this to Skylib for now, until there's a clear need for it. |
rules_nodejs does have an immediate need for this, to unblock usage of arbitrary programs under Bazel on Windows. I'll just copy this code into our repo for now. |
Based on meteorcloudy@ awesome work in bazelbuild/bazel-skylib#217 That PR isn't getting approved for upstream so we vendor it into our own ruleset
Based on meteorcloudy@ awesome work in bazelbuild/bazel-skylib#217 That PR isn't getting approved for upstream so we vendor it into our own ruleset
Based on meteorcloudy@ awesome work in bazelbuild/bazel-skylib#217 That PR isn't getting approved for upstream so we vendor it into our own ruleset
* feat(builtin): introduce copy_to_bin rule Based on meteorcloudy@ awesome work in bazelbuild/bazel-skylib#217 That PR isn't getting approved for upstream so we vendor it into our own ruleset * chore: migrate to copy_file and copy_to_bin
A rule that copies a source file to bazel-bin at the same execroot path.
Eg. <source_root>/foo/bar/a.txt -> /foo/bar/a.txt
We cannot use genrule or copy_file rule to copy a source file to bazel-bin with
the same execroot path, because both rules create label for the output
artifact, which conflicts with the source file label. map_to_output rule
achieves that by avoiding creating label for the output artifact.