Skip to content

[bazel] fix: use nodejs toolchain resolution#1687

Open
nickschaap wants to merge 2 commits intoemscripten-core:mainfrom
nickschaap:schaap/nodejs-toolchain
Open

[bazel] fix: use nodejs toolchain resolution#1687
nickschaap wants to merge 2 commits intoemscripten-core:mainfrom
nickschaap:schaap/nodejs-toolchain

Conversation

@nickschaap
Copy link

@nickschaap nickschaap commented Mar 20, 2026

Uses Bazel toolchain resolution for incorporating the NodeJS toolchain so that correct version of NodeJS is resolved based off execution platform which may differ from the current host platform.

Also marks the npm module_extension as a dev dependency. I'm not sure its used as public API.

@sbc100 sbc100 requested review from DoDoENT and mmorel-35 March 20, 2026 19:55
@sbc100
Copy link
Collaborator

sbc100 commented Mar 20, 2026

When submitting bazel PRs can you prefix the description with [bazel]?

@nickschaap nickschaap changed the title fix: use nodejs toolchain resolution [bazel] fix: use nodejs toolchain resolution Mar 20, 2026
npm = use_extension(
"@aspect_rules_js//npm:extensions.bzl",
"npm",
dev_dependency = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this. Usually, dev dependencies are not transitive, meaning that the NPM extension and everything that depends on it will not be visible to emscripten's users.
I'm not completely sure this is correct, as emscripten does rely on NPM and Node during build execution.
Do you have any proof that this works with NPM being a dev dependency only?

Copy link
Author

@nickschaap nickschaap Mar 21, 2026

Choose a reason for hiding this comment

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

I'm not sure this even works today. When I try to integrate with emsdk from my repo I run into errors during the evaluation of the npm module extension:

ERROR: /Users/schaap/dev/.bazel/output_root/e53c93f6697881c24878eb22525e770a/external/aspect_rules_js~/npm/private/npm_translate_lock_helpers.bzl:661:13: Traceback (most recent call last):
        File "/Users/schaap/dev/.bazel/output_root/e53c93f6697881c24878eb22525e770a/external/aspect_rules_js~/npm/extensions.bzl", line 97, column 39, in _npm_extension_impl
                _npm_translate_lock_bzlmod(module_ctx, mod, attr, exclude_package_contents_config, merged_replace_packages)
        File "/Users/schaap/dev/.bazel/output_root/e53c93f6697881c24878eb22525e770a/external/aspect_rules_js~/npm/extensions.bzl", line 145, column 34, in _npm_translate_lock_bzlmod
                state = parse_and_verify_lock(module_ctx, mod, attr)
        File "/Users/schaap/dev/.bazel/output_root/e53c93f6697881c24878eb22525e770a/external/aspect_rules_js~/npm/private/npm_translate_lock.bzl", line 371, column 45, in parse_and_verify_lock
                helpers.verify_lifecycle_hooks_specified(state)
        File "/Users/schaap/dev/.bazel/output_root/e53c93f6697881c24878eb22525e770a/external/aspect_rules_js~/npm/private/npm_translate_lock_helpers.bzl", line 661, column 13, in _verify_lifecycle_hooks_specified
                fail(msg)
Error in fail: ERROR: pnpm 'onlyBuiltDependencies' configuration required.

Packages that rules_js should generate lifecycle hook actions for must be declared in
'onlyBuiltDependencies'.

As of pnpm v10 'onlyBuiltDependencies' should be configured in the pnpm-workspace.yaml file
alongside other pnpm configuration. See https://pnpm.io/10.x/settings#onlybuiltdependencies
for more information.

In pnpm v9 'onlyBuiltDependencies' is configured in the root package.json
pnpm.onlyBuiltDependencies. The root package.json must be alongside the pnpm-lock.yaml file
and will be automatically detected even if not explicitly added to data attribute.
See https://pnpm.io/9.x/package_json#pnpmonlybuiltdependencies for more information.

When I do bazel fetch --repo @emscripten_npm_linux I'm also running into issues. How are any of these repos used by consumers today? I don't see any labels referencing them in BUILD or bzl files and consumers have no way of accessing these directly since they are not re-exported in any way.

I marked it as a dev_dependency which fixes the issues I'm seeing in my repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. I just tried bazel fetch --repo @emscripten_npm_linux with emsdk 5.0.1 I have on one of my projects and it failed with

ERROR: Fetching some repos failed with errors: No repository visible as '@emscripten_npm_linux' from main repository

I never used NPM from emscripten directly in my projects so I wouldn't know - ATM I only use emscripten toolchain to build C++ code. I'll probably venture into integrating with JS/TS in a couple of weeks or so. I guess I'll then have the same issue as you 🤷🏻 .

Anyway, if it works for you and if it doesn't break anyone else (check CI jobs), I'm OK with this getting merged.

Copy link
Author

Choose a reason for hiding this comment

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

@DoDoENT Actually I think these definitions can be removed entirely since the emscripten_deps repository already includes all the required node_module dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly. I don't know who the original author of Bazel support for emscripten was and why they added this dependency. Maybe @sbc100 knows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would a look at the git blame to find out.

@nickschaap nickschaap requested a review from DoDoENT March 21, 2026 00:48
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.

3 participants