feat: make node toolchain_type public so new toolchains can be added #2591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Enables rules_nodejs to be used in cross-compilation contexts, where
the target architecture is not supported by rules_nodejs.
rules_nodejs's default toolchains are limited via target_compatible_with,
which means they will not work when cross-compiling to Android or iOS
with --platforms for example. With this change, users can define their
own toolchain that is limited by execution platform instead of target
platform.
For example, if you're targeting an unsupported platform, but building
on a Mac, you can place the following in the local WORKSPACE:
register_toolchains("//ts/node_toolchain")
And the following in ts/node_toolchain/BUILD.bazel:
toolchain(
name = "node_toolchain",
exec_compatible_with = [
"@platforms//os:osx",
"@platforms//cpu:x86_64",
],
toolchain = "@nodejs_darwin_amd64_config//:toolchain",
toolchain_type = "@build_bazel_rules_nodejs//toolchains/node:toolchain_type",
)
Closes bazel-contrib#2565
dae
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
2 tasks
dae
added a commit
to ankitects/rules_nodejs
that referenced
this pull request
Apr 7, 2021
This currently works when testing packages/esbuild/test, but not when used via a built package, and I wanted to get an idea of if this is a desired change before proceeding further. The current recommended way to pass the esbuild binary to the rule is to use a select() statement, but that makes a decision based on the target architecture, not the host architecture, and causes builds to fail when a target platform has been set with --platforms that is not one of Mac/Windows/Linux (related: bazel-contrib#2591) By switching to a toolchain, we can specify that each platform's binary requires a given host platform, so if building on a Mac for example, it will invoke the esbuild Mac binary even if a build is targeting another architecture, eg building a native Android/iOS library that needs to bundle esbuild output products. I made some other changes in this PR for discussion - they are not required for toolchain support, but IMHO would be nice to have: - Bundle esbuild_repo.bzl, so users can get easy access to the same esbuild version that the tests are run against. Currently, if you update rules_nodejs after a change like the move to esbuild 0.11, your build will break until you update the local esbuild binaries in your repo to match. The change also makes esbuild behave more like the core nodejs toolchain, which includes some default binary URLs. - Change the script that generates esbuild_repo.bzl to use maybe(), so that users can override the default URLs if they wish. This is not strictly necessary, as the user could define and register their own toolchains instead, but it just makes it a bit easier to override the default values. If there is interest in the toolchain change (and possibly the repo change), I'd appreciate some advice on how to proceed. A toolchains= argument to a rule seems to require an absolute label, so I can't use ":toolchain_type" - it needs to be "//packages/esbuild:toolchain_type" in local development, and something like "@npm//@bazel/esbuild:toolchain_type" when installed via npm. The package.json in examples/esbuild appears to reference npm packages directly, so presumably the examples can't be updated until a new release is made. Is there some trick I can use to replace the npm reference with the files in dist/bin/packages/esbuild/npm_package? I tried updating the package.json file to reference "file:../../dist/bin/packages/esbuild/npm_package", but the build fails, and I'm not sure if that's because it's creating a symlink in the file: case. Is there a recommended way I can test out the changes without having to upload packages to npm? Is this something there'd be interest in including?
12 tasks
alexeagle
approved these changes
Apr 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables rules_nodejs to be used in cross-compilation contexts, where
the target architecture is not supported by rules_nodejs.
rules_nodejs's default toolchains are limited via target_compatible_with,
which means they will not work when cross-compiling to Android or iOS
with --platforms for example. With this change, users can define their
own toolchain that is limited by execution platform instead of target
platform.
For example, if you're targeting an unsupported platform, but building
on a Mac, you can place the following in the local WORKSPACE:
And the following in ts/node_toolchain/BUILD.bazel:
Closes #2565
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information