bazel-skylib icon indicating copy to clipboard operation
bazel-skylib copied to clipboard

Add map_to_output rule

Open meteorcloudy opened this issue 4 years ago • 9 comments

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.

meteorcloudy avatar Nov 15 '19 12:11 meteorcloudy

FYI @alexeagle

meteorcloudy avatar Nov 15 '19 12:11 meteorcloudy

@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?

thomasvl avatar Nov 15 '19 15:11 thomasvl

Indeed, I think we should generally apply some scrutiny to whether a new rule belongs in skylib. In general, skylib should contain "utilities to help Starlark rule writers". There's of course a gray area with certain rules, in that they might effectively help macro writers.

For this particular PR, could we get some context as to who we expect to use this?

c-parsons avatar Nov 15 '19 15:11 c-parsons

Sure, this is for rules_nodejs, they have a need to copy a source file to bazel-bin at the same path, (eg. <source>/foo/bar/a.txt to <bazel-bin>/foo/bar/a.txt). But this is not possible with genrule or the existing copy_file rule.

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.

meteorcloudy avatar Nov 15 '19 15:11 meteorcloudy

More generally this provides a third option for running binaries/tests:

  1. only programs with runfiles awareness can be used with Bazel
  2. rely on runfiles symlink forest and make windows users pass --enable_runfiles
  3. make the output tree complete so programs find their data relative to their PWD

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?) If we vendor to rules_nodejs it would be best for it to land in skylib first like we do for https://github.com/bazelbuild/rules_nodejs/tree/master/third_party/github.com/bazelbuild/bazel-skylib

alexeagle avatar Nov 15 '19 16:11 alexeagle

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 ?

c-parsons avatar Nov 15 '19 17:11 c-parsons

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

alexeagle avatar Nov 16 '19 21:11 alexeagle

I'd suggest not adding this to Skylib for now, until there's a clear need for it.

laurentlb avatar Nov 26 '19 18:11 laurentlb

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.

alexeagle avatar Nov 26 '19 18:11 alexeagle