fix(build): pin opendal linux ABI#1872
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds OpenDAL native package smoke testing and packaging support: a new smoke-test CLI script, an afterPack.js routine to copy platform-specific OpenDAL native packages into unpacked Electron builds, pinned opendal dependency versions, updated sharp install platform config for Linux libc/wasm32, CI workflow verification steps, and corresponding test coverage. ChangesOpenDAL Native Packaging and Verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Script as smoke-opendal-native.js
participant FS as node_modules / resourcesPath
participant Loader as maybeLoadOpendal
CI->>Script: run smoke:opendal:native (--platform, --arch, --resources-path)
Script->>FS: resolve OpenDAL native package dir
FS-->>Script: package.json + main file
Script->>Loader: assert main exists, conditionally require index.cjs
Loader-->>Script: load result or skip (cross-platform)
Script-->>CI: exit 0 (success) or exit 1 (failure)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/smoke-opendal-native.js (1)
131-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
index.cjsbypasses package.jsonmainresolution.
maybeLoadOpendalhardcodesindex.cjsas the opendal entry point instead of resolving it viapackage.json'smainfield the wayassertPackageMainExistsdoes for the native package. This is accurate for the currently pinnedopendal@0.49.2(confirmed it shipsindex.cjs), but silently breaks the smoke check if a future version pin changes the entry filename, since there's no assertion tying this hardcoded path to the actual manifest.♻️ Proposed fix to derive the entry from package.json
function maybeLoadOpendal(opendalDir, platform, arch, label) { if (platform !== process.platform || arch !== process.arch) { console.log( `[OpenDAL Smoke] ${label}: target ${platform}/${arch} differs from host ${process.platform}/${process.arch}; verified file presence only.` ) return } - const opendalEntry = path.join(opendalDir, 'index.cjs') + const opendalEntry = assertPackageMainExists(opendalDir, `${label} opendal`) const requireFromOpendal = createRequire(opendalEntry) requireFromOpendal(opendalEntry) console.log(`[OpenDAL Smoke] ${label}: loaded opendal from ${opendalEntry}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/smoke-opendal-native.js` around lines 131 - 143, The OpenDAL load path in maybeLoadOpendal is hardcoded to index.cjs instead of using the package manifest, which can drift from the actual entry point. Update maybeLoadOpendal to resolve the entry from opendal’s package.json main field, similar to assertPackageMainExists for the native package, and use that resolved entry with createRequire/requireFromOpendal so the smoke test stays aligned with future package changes.scripts/afterPack.js (1)
204-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging when no OpenDAL package resolves for a recognized platform.
When
getOpendalNativePackagesreturns[]for a platform/arch combo that isn't in the switch, the function silently no-ops (Line 208-210). If CI's build matrix later adds a target (e.g. Windows ia32 or a new arch enum value) without updating the switch, the packaged app would ship without the native OpenDAL binary and no build failure would surface it.♻️ Optional: warn on unmapped platform/arch
const packageNames = getOpendalNativePackages(electronPlatformName, arch) if (packageNames.length === 0) { + console.warn( + `No OpenDAL native package mapping for ${electronPlatformName}/${arch}; skipping native copy.` + ) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/afterPack.js` around lines 204 - 231, The copyOpendalNativePackages flow currently returns silently when getOpendalNativePackages() yields no packages, which can hide unmapped platform/arch combinations. Update copyOpendalNativePackages (near the packageNames check) to emit a warning that includes electronPlatformName and arch before returning, so missing OpenDAL mappings are visible in CI without changing the existing no-op behavior.test/main/scripts/afterPack.test.ts (1)
177-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the darwin "universal" OpenDAL branch.
getOpendalNativePackagesinscripts/afterPack.jshas a distinct branch forplatform === 'darwin' && archName === 'universal'that returns two packages (lib-darwin-x64andlib-darwin-arm64). This parameterized test only coversarm64(arch 3) andx64(arch 1) individually; the universal/multi-package copy path isn't exercised here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/afterPack.test.ts` around lines 177 - 220, Add test coverage in afterPack.test.ts for the darwin universal OpenDAL path in getOpendalNativePackages. The current parameterized case only verifies arm64 and x64 separately, so add a new scenario that uses archName "universal" and asserts both OpenDAL native packages are copied into the unpacked node_modules by afterPack. Reference the existing afterPack, seedDarwinNativePrerequisites, and getOpendalNativePackages behavior so the new test clearly exercises the multi-package branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/afterPack.js`:
- Around line 204-231: The copyOpendalNativePackages flow currently returns
silently when getOpendalNativePackages() yields no packages, which can hide
unmapped platform/arch combinations. Update copyOpendalNativePackages (near the
packageNames check) to emit a warning that includes electronPlatformName and
arch before returning, so missing OpenDAL mappings are visible in CI without
changing the existing no-op behavior.
In `@scripts/smoke-opendal-native.js`:
- Around line 131-143: The OpenDAL load path in maybeLoadOpendal is hardcoded to
index.cjs instead of using the package manifest, which can drift from the actual
entry point. Update maybeLoadOpendal to resolve the entry from opendal’s
package.json main field, similar to assertPackageMainExists for the native
package, and use that resolved entry with createRequire/requireFromOpendal so
the smoke test stays aligned with future package changes.
In `@test/main/scripts/afterPack.test.ts`:
- Around line 177-220: Add test coverage in afterPack.test.ts for the darwin
universal OpenDAL path in getOpendalNativePackages. The current parameterized
case only verifies arm64 and x64 separately, so add a new scenario that uses
archName "universal" and asserts both OpenDAL native packages are copied into
the unpacked node_modules by afterPack. Reference the existing afterPack,
seedDarwinNativePrerequisites, and getOpendalNativePackages behavior so the new
test clearly exercises the multi-package branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee5c177c-68e9-4e60-8f0c-571d09ae43e9
📒 Files selected for processing (9)
.github/workflows/build.yml.github/workflows/release.ymlpackage.jsonscripts/afterPack.jsscripts/install-sharp-for-platform.jsscripts/smoke-opendal-native.jstest/main/build/electronBuilderConfig.test.tstest/main/scripts/afterPack.test.tstest/main/scripts/installSharpForPlatform.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pin opendal linux ABI
Summary by CodeRabbit
New Features
Bug Fixes