Introduce runtime_toolchain_type #3859
Open
Conversation
This file contains hidden or 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
guw
requested review from
mattem,
gregmagolan,
alexeagle and
jbedard
as code owners
|
4 tasks
|
This requires follow up fixes in |
According to discussions in bazel-contrib#3854 having two toolchains of the same type for different things is troublesome. It's better to have separate runtime as well as compile toolchains. This commit creates a new runtime_toolchain_type and registers toolchains without execution constraints for this type. Once merged, rules_ts can start consuming the new toolchain type in its js_binary rule to ensure the correct Node for the correct target environment is selected. Fixed [Bug]: Execution toolchain defined without `target_compatible_with` makes it a candidate to selection Fixes bazel-contrib#3854 Work towards bazel-contrib#3795
guw
force-pushed
the
add_runtime_toolchain
branch
from
2bdbe1f to
3a0c8a3
Compare
guw
commented
Aug 23, 2025
|
I really like where this is going. I will review this in detail soon. |
fmeum
reviewed
Aug 23, 2025
| @@ -67,7 +73,6 @@ genrule( | |||
| outs = ["actual1"], | |||
| cmd = "$(NODE_PATH) $(execpath some.js) $@", | |||
| toolchains = ["@nodejs_toolchains//:resolved_toolchain"], | |||
| tools = ["@nodejs_toolchains//:resolved_toolchain"], | |||
There was a problem hiding this comment.
A resolved toolchain target would still be needed with Bazel 7, toolchain resolution for genrules is a Bazel 8 feature.
There was a problem hiding this comment.
But the tests do pass with .bazelversion set to 7.3.1
❯ bazel info release
release 7.3.1
rules_nodejs/e2e/smoke on add_runtime_toolchain [!]
❯ bazel test --toolchain_resolution_debug="@rules_nodejs.+" //:test_genrule
INFO: ToolchainResolution: Performing resolution of @@rules_nodejs~//nodejs:runtime_toolchain_type for target platform @@platforms//host:host
ToolchainResolution: Rejected toolchain @@rules_nodejs~~node~nodejs_linux_amd64//:toolchain; mismatching values: linux, x86_64
ToolchainResolution: Rejected toolchain @@rules_nodejs~~node~nodejs_linux_arm64//:toolchain; mismatching values: linux
ToolchainResolution: Rejected toolchain @@rules_nodejs~~node~nodejs_linux_s390x//:toolchain; mismatching values: linux, s390x
ToolchainResolution: Rejected toolchain @@rules_nodejs~~node~nodejs_linux_ppc64le//:toolchain; mismatching values: linux, ppc
ToolchainResolution: Rejected toolchain @@rules_nodejs~~node~nodejs_darwin_amd64//:toolchain; mismatching values: x86_64
ToolchainResolution: Toolchain @@rules_nodejs~~node~nodejs_darwin_arm64//:toolchain is compatible with target plaform, searching for execution platforms:
ToolchainResolution: Compatible execution platform @@platforms//host:host
ToolchainResolution: All execution platforms have been assigned a @@rules_nodejs~//nodejs:runtime_toolchain_type toolchain, stopping
ToolchainResolution: Recap of selected @@rules_nodejs~//nodejs:runtime_toolchain_type toolchains for target platform @@platforms//host:host:
ToolchainResolution: Selected @@rules_nodejs~~node~nodejs_darwin_arm64//:toolchain to run on execution platform @@platforms//host:host
INFO: ToolchainResolution: Target platform @@platforms//host:host: Selected execution platform @@platforms//host:host, type @@rules_nodejs~//nodejs:runtime_toolchain_type -> toolchain @@rules_nodejs~~node~nodejs_darwin_arm64//:toolchain
INFO: ToolchainResolution: Target platform @@platforms//host:host: Selected execution platform @@platforms//host:host,
INFO: ToolchainResolution: Target platform @@platforms//host:host: Selected execution platform @@platforms//host:host,
INFO: Analyzed target //:test_genrule (63 packages loaded, 3082 targets configured).
INFO: Found 1 test target...
Target //:test_genrule up-to-date:
bazel-bin/test_genrule-test.sh
INFO: Elapsed time: 8.155s, Critical Path: 1.93s
INFO: 9 processes: 6 internal, 3 darwin-sandbox.
INFO: Build completed successfully, 9 total actions
//:test_genrule PASSED in 0.9s
Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
There was a problem hiding this comment.
Weird. @alexeagle Do you know whether genrule merges in files from toolchains?
|
|
||
| for [platform, meta] in PLATFORMS.items(): | ||
| build_content += """ | ||
| toolchain( | ||
| name = "{platform}_toolchain", | ||
| exec_compatible_with = {compatible_with}, | ||
| target_compatible_with = {compatible_with}, # https://github.com/bazel-contrib/rules_nodejs/issues/3854 |
There was a problem hiding this comment.
The real reason for this would be native compilation, right? Could you expand on this in the comment?
There was a problem hiding this comment.
I updated comment. I believe the real reason is to workaround/mitigate deficiencies in js_binary/js_image_oci picking the wrong toolchain with the existing toolchains.
Long term: aspect-build/rules_js#2330
There was a problem hiding this comment.
I hope that isn't just a long term improvement - it looks like a substantial bug fix that only requires a rules_nodejs release and shouldn't be breaking (unless users register custom toolchains)
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.
According to discussions in #3854 having two toolchains of the same type for different things is troublesome. It's better to have separate runtime as well as compile toolchains.
This commit creates a new runtime_toolchain_type and registers toolchains without execution constraints for this type. Once merged, rules_ts can start consuming the new toolchain type in its js_binary rule to ensure the correct Node for the correct target environment is selected.
Fixed [Bug]: Execution toolchain defined without
target_compatible_withmakes it a candidate to selectionFixes #3854
Work towards #3795
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
It tries to remain compatible and support existing consumers.
Other information
This is an alternative to #3800.