Conversation
📝 WalkthroughWalkthroughThis PR adds support for Loongarch64 architecture targets (GNU/Linux and musl variants) to the NAPI release workflow, configures necessary cross-compilation toolchain for GNU/Linux, and updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 4d0b208 in 2 minutes and 13 seconds. Click for details.
- Reviewed
669lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. napi/index.js:369
- Draft comment:
Ensure correct arch string for loongarch64 support; 'process.arch' might be 'loongarch64' instead of 'loong64'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This comment is speculative - it says "might be" without providing evidence that Node.js actually reports 'loongarch64' as a process.arch value. The file is auto-generated by NAPI-RS, so manual changes would likely be overwritten. The comment doesn't provide strong evidence that this is actually an issue - no documentation links, no test cases, no real-world examples. All other architectures in the file use a single canonical name without alternatives. If this were a real issue, it would need to be fixed in the NAPI-RS code generator, not in this generated file. The comment is asking the author to make a change based on speculation rather than demonstrated need. However, it's possible that Node.js does report different arch strings for LoongArch on different systems or versions, and this could be a legitimate compatibility concern. The commenter might have domain knowledge about LoongArch systems that I lack. Even if this is a legitimate concern, the comment violates the rule about not commenting on auto-generated code without strong evidence. The file header clearly states it's auto-generated by NAPI-RS. Any fix would need to be made upstream in the generator, not in this file. Additionally, the comment is speculative ("might be") rather than definitive, which violates the rule against speculative comments. This comment should be deleted. It's speculative without strong evidence, and it's commenting on auto-generated code where manual changes would be inappropriate. If this is a real issue, it needs to be addressed in the NAPI-RS code generator, not in this generated file.
2. napi/index.js:81
- Draft comment:
Consider refactoring the repetitive binding version check logic into a helper, to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. napi/index.js:575
- Draft comment:
Review the error-chaining in the reduce call; modifying 'cur.cause' may not produce the intended nested error chain. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. .github/workflows/release-napi.yml:107
- Draft comment:
Typographical error: 'apt-fet' should be corrected to 'apt-get'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_L6uDecx0v3H10GTm
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
.github/workflows/release-napi.yml
Outdated
| target: loongarch64-unknown-linux-gnu | ||
| build: |- | ||
| sudo apt-get update && | ||
| sudo apt-fet install gcc-13-loongarch64-linux-gnu g++-13-loongarch64-linux-gnu -y && |
There was a problem hiding this comment.
Typo: use apt-get (not apt-fet) in the loongarch64 GNU build step.
| sudo apt-fet install gcc-13-loongarch64-linux-gnu g++-13-loongarch64-linux-gnu -y && | |
| sudo apt-get install gcc-13-loongarch64-linux-gnu g++-13-loongarch64-linux-gnu -y && |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/release-napi.yml:
- Around line 103-111: The CI job for target "loongarch64-unknown-linux-gnu"
contains a typo in the build script: replace the misspelled command "apt-fet"
with "apt-get" in the build step so the package installation line runs
correctly; update the build block where the target/environment variables are set
(the block that exports CC_loongarch64_unknown_linux_gnu,
CXX_loongarch64_unknown_linux_gnu, and
CARGO_TARGET_LOONGARCH64_UNKNOWN_LINUX_GNU_LINKER) to use the corrected
"apt-get" command.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release-napi.ymlnapi/index.jspackage.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-05T18:52:30.363Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Applied to files:
package.jsonnapi/index.js
📚 Learning: 2025-06-10T13:53:02.983Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
Applied to files:
napi/index.js
📚 Learning: 2025-03-29T04:14:03.715Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Applied to files:
napi/index.js
📚 Learning: 2025-04-21T08:11:41.469Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Applied to files:
napi/index.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/workflows/release-napi.yml (2)
112-114: LGTM!The loongarch64-unknown-linux-musl target correctly uses the
-xflag for Zig cross-compilation, consistent with other musl targets in this workflow.
137-140: Zig version 0.14.0 is confirmed released and stable.Zig 0.14.0 (released March 5, 2025) includes expanded target support for loongarch64-linux and ships with musl v1.2.5 for musl-based builds. The version specified in the workflow is correct and suitable for the intended musl-target builds.
package.json (2)
31-31: LGTM!Good upgrade from alpha (3.0.0-alpha.92) to stable (3.3.1) release of @napi-rs/cli.
70-71: LGTM!The new loongarch64 targets correctly align with the CI workflow matrix entries added in the release workflow.
napi/index.js (3)
1-4: Auto-generated file - limited review scope.This file is auto-generated by NAPI-RS. The formatting inconsistencies (e.g., lines 111-143) are artifacts of the code generator and not actionable in this PR.
369-402: LGTM!The loong64 architecture support follows the established pattern used by other architectures in this file, with proper musl/glibc detection and version enforcement checks.
574-579: Error chaining implementation is correct.The
reduce()call safely chains error causes. The guardloadErrors.length > 0on line 569 ensures the array is never empty when this code executes, avoiding the reduce-on-empty-array pitfall.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
napi/index.js (1)
81-86: Version checking mechanism is well-designed.The version checking for native bindings adds a valuable safeguard against version mismatches. The opt-in enforcement via
NAPI_RS_ENFORCE_VERSION_CHECKensures backward compatibility while allowing strict validation when needed.Minor observation about the hardcoded version
The version string
'1.11.1'appears in approximately 30 locations throughout the file. Since this is auto-generated code (line 4 comment), this is expected. However, if modifying the generator, consider extracting this to a constant at the top of the file for easier maintenance during version bumps.This is purely informational since manual edits to auto-generated files would be overwritten.
Also applies to: 97-102, 118-123, 134-139, 151-156, 167-172, 186-191, 202-207, 218-223, 238-243, 254-259, 275-280, 291-296, 309-314, 325-330, 343-348, 359-364, 377-382, 393-398, 411-416, 427-432, 444-449, 460-465, 480-485, 496-501, 512-517
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release-napi.ymlnapi/index.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release-napi.yml
- package.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-10T13:53:02.983Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
Applied to files:
napi/index.js
📚 Learning: 2025-03-29T04:14:03.715Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Applied to files:
napi/index.js
📚 Learning: 2025-04-21T08:11:41.469Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Applied to files:
napi/index.js
📚 Learning: 2025-06-05T18:52:30.363Z
Learnt from: JounQin
Repo: unrs/unrs-resolver PR: 136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Applied to files:
napi/index.js
🧬 Code graph analysis (1)
napi/index.js (3)
napi/resolver.wasi.cjs (1)
require(9-9)src/lib.rs (1)
require(333-348)napi/wasi-worker.mjs (2)
require(7-7)require(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
napi/index.js (4)
369-402: LGTM! Loongarch64 support correctly implemented.The loongarch64 architecture support (Node.js arch name
loong64) follows the established pattern for other architectures, with proper musl/gnu libc detection and fallback to npm packages with version checking. This aligns with the PR objectives and is consistent with the targets added to the release workflow.
111-143: MINGW detection correctly implemented.The MINGW detection for Windows x64 properly distinguishes between GNU and MSVC builds using safe optional chaining. The nested structure ensures the correct binding is loaded based on the runtime environment.
532-557: WASI fallback handling improved with proper error chaining.The WASI binding logic now includes proper error tracking with
wasiBindingErrorand cascading fallbacks between local and npm packages. The new forced error mode (NAPI_RS_FORCE_WASI === 'error') provides explicit failure when WASI is required but unavailable, with proper cause chaining for debugging.
574-579: Error aggregation provides excellent debugging context.The error chaining using
reducecreates a linked list of all load failures, with each error'scauseproperty pointing to the previous failure. This provides comprehensive debugging information when native bindings cannot be loaded.
|
Can we increase support for the loongarch64 architecture? I can build a .node file for loongarch64 using Docker image of ubuntu:latest and versions of Zig 0.14.0 and @ napi-rs/cli 3.3.1
Important
Add loongarch64 architecture support and update version handling for native bindings.
loongarch64-unknown-linux-gnuandloongarch64-unknown-linux-musltargets in.github/workflows/release-napi.ymlandpackage.json.requireNative()innapi/index.jsto handleloong64architecture.requireNative()innapi/index.js.@napi-rs/clito version3.3.1inpackage.json.This description was created by
for 4d0b208. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.