Remove runfiles.

[Globegitter]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:
  Simplifying the setup/build of nodejs_binary and nodejs_test, which simplifies work for Initial Toolchain support #645 and Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

What is the current behavior?

Every nodejs_binary gets the _node_runfiles filegroup added, which adds the real node binary as well as npm, yarn and some other libraries. Further the implicit node binary dependency is a wrapper script which adds the bazel managed node to the PATH.

What is the new behavior?

For nodejs_binary the wrapper script target was exchanged with the real node binary and the functionality of the wrapper script, setting the PATH was added to the node_loader.js. The runfiles are entirely removed as an implicit dependency as binaries can run fine without these now.

These means we only depend on one single file to execute our nodejs_binary, that should make things a little faster as less things will need to be symlinked but also reduce size of docker containers for most and it will simplify things for the toolchain PR as now it only has to worry about providing one single file target.

Does this PR introduce a breaking change?

• Yes
• No

People that depended on the fact that yarn or node was included as an implicit dependency would now have to add @nodejs//:node_runfiles explicitely to their data dependencies.

[gregmagolan]

LGTM. Really nice simplification. Thanks Markus.

My two concerns were:

1. @nodejs//:yarn, @nodejs//:npm, @nodejs//:bin/yarn, @nodejs//:bin/npm still need to use the /bin/node & /bin/node.cmd entry points which still sets the PATH since those code paths don't use the launcher

2. RBE still works for nodejs_binary which looks like it will since the node binary is still in runfiles

From the looks of it both are good

Is it safe to remove

filegroup(
  name = "node_runfiles",
  srcs = [
    ${nodejsSrcFiles.map(f => `"${NODE_DIR}/${f}",`).join('\n    ')}
    ${yarnSrcFiles.map(f => `"${YARN_DIR}/${f}",`).join('\n    ')}
  ],
)

with this or is there still someone downstream who would use it besides nodejs_binary?

[Globegitter]

@gregmagolan
About 1, yeah I left the original script there on purpose as the same target for that. For 2 yes RBE still works and it is actually being tested in buildkite CI. I did have a failure about that to start off with.

Yeah I was not sure why anyone would depend on the runfiles downstream - but that is why I left them, because I don't know. But I would be very happy to remove them and we can always bring it back if someone needs them for some reason.

[Globegitter]

Done - I kept it in 2 commits. One to address the comment. The other to remove the runfiles.

[Globegitter]

@alexeagle squashed.

[alexeagle]

I rebased around the merge conflict and force-pushed to your branch, FYI. will merge now if green

[Globegitter]

@alexeagle anything blocking this now from getting merged?