[bazel] fix: use nodejs toolchain resolution#1687
[bazel] fix: use nodejs toolchain resolution#1687nickschaap wants to merge 2 commits intoemscripten-core:mainfrom
Conversation
|
When submitting bazel PRs can you prefix the description with |
| npm = use_extension( | ||
| "@aspect_rules_js//npm:extensions.bzl", | ||
| "npm", | ||
| dev_dependency = True, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@DoDoENT Actually I think these definitions can be removed entirely since the emscripten_deps repository already includes all the required node_module dependencies.
There was a problem hiding this comment.
Possibly. I don't know who the original author of Bazel support for emscripten was and why they added this dependency. Maybe @sbc100 knows?
There was a problem hiding this comment.
I would a look at the git blame to find out.
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.