Skip to content
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: update rules_nodejs and migrate to rules_js #1436

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

eric-higgins-ai
Copy link
Contributor

@eric-higgins-ai eric-higgins-ai commented Aug 18, 2024

This finishes the work started in #1388 by fixing CI. It avoids a breaking change by:

  • Using the latest rules_js 1.x.x version, instead of updating to rules_js 2 (which removes support for bazel 5).
  • Copying the contents of rules_js_dependencies instead of calling it, as the call would need to be added by users in their WORKSPACE files

Context from the previous PR:

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.

Similarly, it's not possible to update to rules_js v2 in a workspace that also references emsdk.

@eric-higgins-ai eric-higgins-ai marked this pull request as draft August 18, 2024 18:44
@eric-higgins-ai eric-higgins-ai changed the title bazel: migrate to rules_js bazel: update rules_nodejs and migrate to rules_js Aug 18, 2024
@eric-higgins-ai eric-higgins-ai marked this pull request as ready for review August 18, 2024 19:32
@eric-higgins-ai
Copy link
Contributor Author

@walkingeyerobot bump on a review here

@walkingeyerobot
Copy link
Collaborator

Apologies, this slipped past me somehow. Thank you for pinging me on this, and thank you for the contribution.

I'm unfamiliar with aspect-build and a bit hesitant to accept dependencies. I'll spend some time this week reading their code and docs and such to get a little familiar with it before accepting this PR. It's very likely I end up approving this after learning a bit more about it.

@walkingeyerobot walkingeyerobot enabled auto-merge (squash) December 5, 2024 07:58
Copy link
Collaborator

@walkingeyerobot walkingeyerobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aspect is fine to use

@walkingeyerobot walkingeyerobot merged commit c7d7853 into emscripten-core:main Dec 5, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants