-
-
Notifications
You must be signed in to change notification settings - Fork 151
refactor: adopt new NodeJS runtime toolchain #2330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
nice, verified this fixes this case too #2328 |
1390f08 to
3eb24e8
Compare
|
I am left with the following issue when building: |
3eb24e8 to
f997478
Compare
f997478 to
6e15cc8
Compare
a9d9687 to
dfeafdb
Compare
7bdc6bd to
6fc0597
Compare
|
@alexeagle We are down to one failing test. This looks like a side effect caused by |
6fc0597 to
60c74e5
Compare
|
@guw would you be able to rebase this and target the In the |
js_binary should use the new runtime toolchain to avoid execution toolchain being leaked into target environments (eg., js_image_oci) See bazel-contrib/rules_nodejs#3854
60c74e5 to
9ea18f2
Compare
|
@jbedard here you go |
|
Great, thanks! 👍 Any ideas about the |
|
Do you know what the |
|
Unfortunately not. But reading through the code, it seems to expect a specific NodeJS version, which is only setup via |
|
That test is green on the |
|
Yes. The tests expects a very specific NodeJS version. I do believe - however - that it should be fetched from somewhere else because of the "vendored" name. |
|
I think previously the register_toolchains registered all in //toolchains/... Why does your PR change that? |
|
@jbedard I think I know what's going on. The toolchain registrations are only for compile/transpile (aka. build) actions. For tests the runtime toolchain is being used. However, the test does not register a runtime toolchain. I'll try to fix that. |
e7fd609 to
844f9d0
Compare
jbedard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 👍



js_binaryshould use the new runtime toolchain to avoid execution toolchain being leaked into target environments (eg.,js_image_oci)See bazel-contrib/rules_nodejs#3854
Depends on:
Changes are visible to end-users: yes
Test plan