[bazel] fix: use nodejs toolchain resolution

[nickschaap]

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]

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

[DoDoENT]

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?

[nickschaap]

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.

[DoDoENT]

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.

[nickschaap]

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

[DoDoENT]

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

[sbc100]

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

[nickschaap]

@cburchert Looks like you had originally added these. Do know if these are planned to be used in the future?

[stevenlr]

Hi, I noticed that the issue described by @nickschaap in #1687 (comment) resembles what I described in #1684, though more generally relating to newer versions of pnpm, it just so happens that in my case it was rules_js that forced me to upgrade to pnpm 10.

Although not really related to this PR, elimitating the npm_translate_lock extension calls (if they are indeed unnecessary) would eliminate that issue.

If that helps, I can run a few tests tomorrow on our project where we do both C++ compilation with Emscripten, and JS/TS stuff with npm dependencies, with an edited version of emsdk and see if things break (I doubt it would).

Update: can confirm that things work fine without the npm_translate_locks.

[nickschaap]

I went ahead and removed these completely since they aren't being used or doing anything today.

[DoDoENT]

I also vote to remove this. If someone misses it, they can raise an issue, and we will revisit it then.

[sbc100]

Is this change good to land now?

[nickschaap]

@sbc100 Yea should be good to go