-
Notifications
You must be signed in to change notification settings - Fork 694
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
bazel: bump rules_nodejs and migrate to rules_js #1388
base: main
Are you sure you want to change the base?
Conversation
I'm generally in favor of upgrading here but this causes the bazel CI tests to fail. |
@keith, |
I think we probably need to fix the CI here first |
@walkingeyerobot I would also like to upgrade rules_nodejs and nodejs since embuilder is complaining about the current version used in the Bazel workspace and this might be the source of the errors that I am seeing in #1405
Should I take over this PR or perhaps open a new one? I don't think I can push commits here unless I am a "maintainer" to this repository, right? |
You're welcome to open a new one; I am unsure how to transfer ownership of a PR, so new one seems easiest. |
opened a new PR to fix CI #1436 |
Bazel's Node.js dependency comes from rules_nodejs. Previously, bazel/deps.bzl was using rules_nodejs 5.8.0, released in 2022 and only supported Node.js toolchains up to 18.12.1.
This PR bumps rules_nodejs to latest 6.1.1. It also replaces build_bazel_rules_nodejs with rules_js, since
npm_install
that bazel/emscripten_deps.bzl used was deprecated. The README of rules_nodejs now recommends migrating to rules_js for everything other than the Node.js toolchain: (bazel-contrib/rules_nodejs@371e8ca)Impetus
Our repo builds with Bazel and uses Emscripten and Node.js. Tried to upgrade Node.js 18 to Node.js 20 and saw that emsdk didn't support rules_nodejs 6+ in the same workspace.