Js/attempt package fix #47
Conversation
…ilding Node binaries
WalkthroughCI workflow broadened to macOS 15, Node 22–24, and multi-arch matrices; Docker Zig image switched to Alpine; yarn bumped to 3.8.7 with musl support; native binding version checks updated to expect 0.7.0-internaltest.0 across platforms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GitHub as "GitHub Actions"
participant Matrix as "Matrix Runner\n(Node x Arch)"
participant Docker as "Docker (Zig Alpine)"
participant Test as "Bindings Tests"
participant Art as "Artifacts"
participant NPM as "Publish step"
Note over GitHub,Matrix: CI triggers on push/pr
GitHub->>Matrix: Start jobs (Node 22/24 × x64/arm64 × OSes)
Matrix->>Docker: Run build & bindings tests\n(set -e, simplified volumes)
Docker->>Test: Execute platform-specific tests
Test-->>Art: Upload test artifacts (per-arch)
Art-->>GitHub: Artifacts available to publish job
GitHub->>NPM: Publish using Node 22, per-arch artifacts
Note right of NPM: publish completes (or fails)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish_to_npm.yaml (1)
57-58: Remove duplicateyarn set version 3command.Line 57 and 58 both execute
yarn set version 3in the aarch64 Darwin build. This is redundant and should be removed.Apply this diff to fix the duplication:
- host: macos-latest target: aarch64-apple-darwin build: | yarn set version 3 - yarn set version 3 yarn build --target aarch64-apple-darwin strip -x *.node
🧹 Nitpick comments (3)
.github/workflows/publish_to_npm.yaml (3)
256-267: Consider more robust musl detection method.The check for musl at line 260–261 reads
/usr/bin/lddas text and searches for "musl" substring. This is fragile because the file content could change or the string might not appear in expected locations. A more reliable approach is to callldd --versionand parse its output.Apply this improved detection:
- const ldd = fs.readFileSync(\"/usr/bin/ldd\", \"utf-8\"); - console.log(\"ldd contains musl:\", ldd.includes(\"musl\")); + const { execSync } = require(\"child_process\"); + try { + const lddVersion = execSync(\"ldd --version\", { encoding: \"utf-8\" }); + console.log(\"musl detected:\", lddVersion.includes(\"musl\")); + } catch (e) { + console.log(\"ldd --version failed:\", e.message); + }
26-35: Consider addingset -eto native build steps for consistency.The Docker-based builds (Ubuntu) use
set -eto ensure any command failure stops the build, but macOS and Windows builds do not. For consistency and to prevent partial build failures (e.g., ifyarn buildfails butstripstill executes), consider addingset -eto these builds as well.Apply this diff to the macOS x64 Darwin build as an example:
- host: macos-14 target: x86_64-apple-darwin build: | + set -e yarn set version 3 rustup target install x86_64-apple-darwin yarn build --target x86_64-apple-darwin strip -x *.nodeApply the same pattern to the Windows and aarch64-apple-darwin builds (lines 32–35, 56–60, 73–75).
Also applies to: 56-60, 73-75
38-38: Consider pinning NAPI-RS image versions for reproducibility.The docker images use floating tags (
lts-debian,lts-alpine) without version pins. While typically stable, this can lead to non-deterministic builds if the base images change. Consider pinning to specific digests or minor versions for production deployments.Example for line 38:
- docker: ghcr.io/napi-rs/napi-rs/nodejs-rust:lts-debian + docker: ghcr.io/napi-rs/napi-rs/nodejs-rust:lts-debian@sha256:...You can retrieve the digest by inspecting the image repository.
Also applies to: 47-47, 63-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.yarn/releases/yarn-3.8.7.cjsis excluded by!**/.yarn/**
📒 Files selected for processing (10)
.github/workflows/publish_to_npm.yaml(12 hunks).yarnrc.yml(1 hunks)index.js(26 hunks)npm/darwin-arm64/package.json(1 hunks)npm/darwin-x64/package.json(1 hunks)npm/linux-arm64-gnu/package.json(1 hunks)npm/linux-x64-gnu/package.json(1 hunks)npm/linux-x64-musl/package.json(1 hunks)npm/win32-x64-msvc/package.json(1 hunks)package.json(2 hunks)
🔇 Additional comments (14)
npm/darwin-arm64/package.json (1)
3-3: LGTM! Version bump is consistent with the internal test release.The pre-release version format follows semantic versioning conventions correctly.
package.json (2)
4-4: LGTM! Version bump aligns with platform-specific packages.The internal test version is consistent across all package manifests.
45-45: Yarn 3.8.7 is confirmed as a stable release and is appropriate for production use.Yarn 3.8.7 was officially released on December 4, 2024, and is available as a stable version. It can be locked in the project using the codebasemanifest entry
"packageManager": "yarn@3.8.7".npm/linux-arm64-gnu/package.json (1)
3-3: LGTM! Version bump is consistent.The change aligns with the coordinated internal test release across all platforms.
.yarnrc.yml (2)
6-6: LGTM! Yarn path updated to match package.json.The Yarn version upgrade from 3.8.1 to 3.8.7 is consistent with the packageManager field in package.json.
3-4: I'll verify the review comment by checking the actual state of the.yarnrc.ymlfile and searching for musl-related configurations in the codebase.
<function_calls>
#!/bin/bashCheck the actual content of .yarnrc.yml
echo "=== Checking .yarnrc.yml content ==="
if [ -f ".yarnrc.yml" ]; then
cat -n ".yarnrc.yml"
else
echo "File not found"
fiecho -e "\n=== Searching for musl-related configurations ==="
Search for musl references in the repository
rg -i "musl" --type yaml --type json --type js -C 3
echo -e "\n=== Checking package.json for napi targets ==="
if [ -f "package.json" ]; then
grep -A 15 '"targets"' package.json || echo "No targets found in package.json"
fiecho -e "\n=== Listing npm directory ==="
if [ -d "npm" ]; then
find npm -type f -name "package.json" | head -20
fi
</function_calls>index.js (1)
80-81: LGTM! All version checks consistently updated.The version enforcement checks across all platform bindings (Android, Windows, macOS, FreeBSD, Linux variants, and OpenHarmony) have been correctly updated to expect
0.7.0-internaltest.0. This auto-generated code maintains the proper version consistency with the updated package manifests.Also applies to: 96-97, 117-118, 133-134, 150-151, 166-167, 185-186, 201-202, 217-218, 237-238, 253-254, 274-275, 290-291, 308-309, 324-325, 342-343, 358-359, 376-377, 392-393, 410-411, 426-427, 443-444, 459-460, 479-480, 495-496, 511-512
npm/win32-x64-msvc/package.json (1)
3-3: LGTM! Version bump is consistent.The Windows x64 MSVC package version aligns with the internal test release.
npm/linux-x64-musl/package.json (1)
3-3: LGTM! Version bump is consistent.The Linux x64 musl package version matches the coordinated internal test release.
npm/darwin-x64/package.json (1)
3-3: LGTM! Version bump is consistent.The macOS x64 package version completes the coordinated version update across all platform-specific packages.
npm/linux-x64-gnu/package.json (1)
3-3: Version bump is consistent across platform-specific packages.The prerelease version
0.7.0-internaltest.0follows proper semver conventions and aligns with the PR objective of an internal test release. No concerns here..github/workflows/publish_to_npm.yaml (3)
130-130: Docker options simplified with GitHub Actions cache fallback.The removal of Docker volume mounts for cargo cache/registry is acceptable because the GitHub Actions cache (lines 93–102) handles build artifact caching separately. This simplification aligns with using GitHub's caching infrastructure for faster builds across runs.
121-121: Verify Node 20 is required for x86 Windows or a conservative choice.Line 121 uses Node 20 for
i686-pc-windows-msvcwhile the main build uses Node 22. This may indicate a genuine compatibility issue with Node 22 on 32-bit Windows, or it could be a conservative default.If Node 22 is supported on i686, consider aligning with the main build version for consistency. If there's a known incompatibility, please document it in a comment within the workflow.
150-172: Architecture matrix expansion is well-structured.The test matrix now explicitly specifies architectures (x64, arm64) for macOS and Windows targets, enabling proper native builds via
setup-node's architecture parameter. This aligns with the PR's goal of expanded platform coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish_to_npm.yaml (1)
57-58: Remove duplicateyarn set version 3command.Line 57 and 58 both run
yarn set version 3—this is redundant and will cause yarn to unnecessarily reinitialize the version. Keep only one.Apply this diff to remove the duplicate:
- host: macos-latest target: aarch64-apple-darwin build: | yarn set version 3 - yarn set version 3 yarn build --target aarch64-apple-darwin strip -x *.node
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/publish_to_npm.yaml(12 hunks)
🔇 Additional comments (4)
.github/workflows/publish_to_npm.yaml (4)
129-129: Verify simplified Docker options are sufficient.The Docker invocation options were simplified from an unspecified prior state to just
--user 0:0 -v ${{ github.workspace }}:/build -w /build. Ensure:
- Build process can write to
/builddirectory with UID 0- No permission issues or missing mounts occur (e.g., cargo cache, SSH keys if needed)
- Environment variables needed by build are still inherited
Since the previous Docker options are not visible in the diff, please verify that the simplified options don't omit any critical mounts or environment variables that the NAPI build pipeline requires.
149-161: Architecture parameters are correctly defined and propagated across all test matrix entries.Verification confirms:
- All three platform entries (macos-14, macos-latest, windows-latest) have explicit
architecturefields (x64, arm64, x64 respectively)- The
setup-node@v4action correctly references${{ matrix.settings.architecture }}on line 171- No entries are missing the architecture specification, preventing any null/undefined values
- The matrix will generate 9 test combinations (3 platforms × 3 Node versions: 20, 22, 24)
249-252: The Docker command syntax is valid and will execute correctly, though thesh -cwrapper is redundant.The YAML literal block (
|) syntax is valid, and the shell command will execute properly. The leading whitespace and newlines within the single-quoted string are handled correctly by the shell parser—sh -cinterprets the command and executesyarn testas intended.However, there's an unnecessary inconsistency with the
test-linux-x64-gnu-bindingjob (line 215), which runs the same docker invocation without thesh -cwrapper:run: docker run --rm -v $(pwd):/build -w /build node:${{ matrix.node }}-slim yarn testThe musl test could use the same single-line format for consistency:
run: docker run --rm -v $(pwd):/build -w /build node:${{ matrix.node }}-alpine yarn testThe
sh -cwrapper adds unnecessary complexity without functional benefit. Both approaches work, but consistency is preferable.
38-38: Docker image pairing is correct and follows napi-rs best practices.The workflow appropriately matches Docker images to target architectures:
- Line 38 (Debian image) → x86_64-unknown-linux-gnu (glibc target) ✓
- Line 47 (Alpine image) → x86_64-unknown-linux-musl (musl target) ✓
- Line 63 (Debian ARM64 image) → aarch64-unknown-linux-gnu (glibc target) ✓
The
ghcr.io/napi-rs/napi-rs/nodejs-rust:lts-alpineandlts-debianimages are official napi-rs build images maintained at https://github.com/napi-rs/napi-rs. They are publicly available, regularly maintained, and include all required build tooling (Rust, build-essential, etc.). Using Alpine for musl targets and Debian for glibc targets is the recommended approach—Alpine's musl libc aligns with the musl target, while Debian's glibc aligns with gnu targets. Build output will not be negatively impacted because the image-to-target pairing is architecturally correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish_to_npm.yaml (1)
24-30: Usemacos-15-intelrunner for x86_64 builds;macos-15is arm64 and cannot natively build x86_64 binaries.macOS 14 and 15 are arm64, while macOS 13 is x86_64. The current configuration attempts to build
x86_64-apple-darwinbinaries on an arm64 runner, which will fail. For x86_64 (Intel) environments, use themacos-15-intellabel, which is available until August 2027.Apply this diff to fix the runner selection:
- - host: macos-15 + - host: macos-15-intel target: x86_64-apple-darwin
♻️ Duplicate comments (1)
.github/workflows/publish_to_npm.yaml (1)
149-157: Critical architecture incompatibility:macos-15(arm64) cannot execute x64 Node binaries.This is the same issue flagged in the previous review (line 171 in older code). macOS 15 is arm64, so specifying
architecture: x64does not enable x86_64 binary execution on GitHub-hosted runners. Thesetup-node@v4action will download an x64 Node distribution, but it cannot run x86_64 binaries on arm64 hardware without Rosetta 2 support, which is not available on GitHub-hosted runners.To test x86_64 binaries, use an Intel macOS runner. Use the
macos-15-intellabel for x86_64 (Intel) environments.Apply this diff to fix the x86_64 test entry:
- host: macos-15 target: x86_64-apple-darwin - architecture: x64 + # Changed to macos-15-intel; remove architecture since it's nativeThen update the host label:
- - host: macos-15 + - host: macos-15-intel target: x86_64-apple-darwin - architecture: x64The arm64 test entry (lines 152–154) is correct and should remain as-is.
🧹 Nitpick comments (1)
.github/workflows/publish_to_npm.yaml (1)
58-58: Remove duplicateyarn set version 3command on line 58.The same command appears on line 57, making line 58 redundant.
Apply this diff to remove the duplicate:
build: | yarn set version 3 - yarn set version 3 yarn build --target aarch64-apple-darwin
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/publish_to_npm.yaml(12 hunks)
🔇 Additional comments (3)
.github/workflows/publish_to_npm.yaml (3)
40-40: ✓ Good:set -eadded to docker build steps for early exit on failure.Using
set -ein bash scripts ensures that the build exits immediately on the first error, preventing silent failures downstream. This is a solid improvement to build reliability.Also applies to: 49-49, 65-65
47-47: ✓ Docker image switch from Debian to Alpine is appropriate.Using
lts-alpinefor musl builds aligns with the musl libc variant and reduces image size.
159-161: ✓ Expanded Node version matrices (20, 22, 24) align with modern Node LTS support.Testing against multiple Node versions improves compatibility assurance. The expansion to Node 22 and 24 is a good practice.
Also applies to: 193-194, 225-226, 262-263
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.