Initial toolchain support

[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:

What is the current behavior?

Nodejs is downloaded for each platform but does not support toolchains, so does e.g. also not support cross compilation or building of docker images on macOS.

Issue Number: First part of #396

What is the new behavior?

Switches out nodejs to be fetched using platform specific toolchains.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[Globegitter]

CI error seems unrelated:

ERROR: Analysis of target '//:test' failed; build aborted: no such package '@remote_coverage_tools//': java.io.IOException: Error downloading [https://mirror.bazel.build/bazel_coverage_output_generator/releases/coverage_output_generator-v1.0.zip] to /home/circleci/.cache/bazel/_bazel_circleci/7f4dae65be8a29efc03e2c4af7f6dd90/external/remote_coverage_tools/coverage_output_generator-v1.0.zip: connect timed out

[Toxicable]

We've been seeing this flake a lot in our CI internally recently.
Kinda good to see it's not isolated.

[Globegitter]

The only thing missing here now is docs as well as some specific tests. Not sure how to test the cross Compilation though. But the way one could build a docker image on mac (without native dependencies) is via: bazel run --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //app

And for docs, should I just add something to the main readme? Or to the docs to nodejs_binary in code? Or both? Or maybe we want to link to a completely new doc specifically on toolchains and cross compilation?

[Globegitter]

Added some docs now to the main README. I also had more of a look at starlark testing and I think we could utilise this (see https://docs.bazel.build/versions/master/skylark/testing.html#using-a-test-target) to also add some tests. But given the impact of this PR, I wonder if we could just try and merge this asap and then I can do a follow-up PR just for tests and maybe even adding a new doc for advanced use-cases like creating a custom toolchain etc.

[alexeagle]

Here's an idea for a simple way to test it:

build a docker_image (do we even have an example or an e2e that uses docker_nodejs_image?) and assert that the node binary contained in the image starts with the Linux binary header bytes (ELF64 or something?). Then we don't have to actually have a docker daemon on the machine where the test runs, but can still ensure that mac/win builds produce a docker image using the changes

another way to test it that's truer to what users do is to enable RBE for our Mac/Win builds on buildkite. this would fail without the change. Might need some help from @meteorcloudy if it gets hard to set this up

[alexeagle]

I'm fine with augmenting the docs in a separate change but I'd like to have at least a basic regression test in place given the pace of development in the repo

[Globegitter]

We currently do not have any docker tests. I was thinking that as well but then we add a circular dependency on rules_docker as they already depend on rules_nodejs. I guess in theory it is fine as it is just a dev dependency. WDYT @alexeagle

But yeah I will also have a look into the starlark tests and see if we can utilise those for these purposes. The other thing we could do is just get the sha from all the different node binaries and compare that in the test.

[Globegitter]

Interesting, also seeing some toolchain references in the docs here: https://github.com/bazelbuild/bazel-skylib/blob/master/docs/unittest_doc.md like unittest_toolchain and register_unittest_toolchains so maybe there is unittest support for toolchains. Will investigate more.

[Globegitter]

Turns out that is just toolchain setup for unittest itself and not to be used for unit testing. Having played around with it a bit, what it seems we could test via the unit testing is that with the different platform flags we are getting the nodejs binary from different external repositories. Would that be enough as a first round of tests? Then we can still see about doing a full e2e test scenario as a follow-up. Maybe it is also possible to hear from other rules maintainers how they test this functionality.

[thesayyn]

When this will be merged to upstream?

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

[Globegitter]

@gregmagolan Currently target_tool_path is actually not plumbed through to node_repositories, which is probably what we would want to do and it is not that much effort, but we should probably agree on the API for it and how far we go. As in, how can we let the user specify a different path for a different platform? Should accept some form of dict, where we map os to path? And should we also provide the user with a way an easy way to specify take node on PATH? Also do we then want to start supporting taking npm and yarn from path? Or do we still want do download that in all cases?

Given all that my thinking was to really make this officially supported only in a follow-up PR.

[Globegitter]

Seems CI is passing again - the building_karma_package step fails on a dependency download.

[gregmagolan]

Replaced by #898