Repository navigation
fix(cli): ship onnxruntime-node + tokenizers natives in linux-x64 tarball - #3921
Conversation
…ux-x64 tarball
The published v0.2.0 linux-x64 tarball crashed on `superset start` with:
Error: Cannot find module './tokenizers.linux-x64-gnu-xcjh3jwk.node'
Require stack: lib/host-service.js
Root cause: Bun.build was statically processing native-loader requires
inside @anush008/tokenizers/index.js (and onnxruntime-node) and emitting
content-hashed asset references in host-service.js. The matching .node
files weren't being copied alongside the bundle, so the require failed
at runtime.
Fix mirrors how better-sqlite3, node-pty, @parcel/watcher, libsql are
already handled:
- host-service/build.ts: mark onnxruntime-node and @anush008/tokenizers
(+ platform variants) as external. Pure-JS wrappers (fastembed,
@mastra/fastembed) keep getting bundled — they only have peer-dep
references that the runtime require() resolves from lib/node_modules.
- cli/scripts/build-dist.ts: add onnxruntime-node and @anush008/tokenizers
to NATIVE_PACKAGES so they're copied whole. Add the platform-specific
@anush008/tokenizers-{darwin-universal,linux-x64-gnu} subpackages to
TARGET_NATIVE_PACKAGES per target.
Verified end-to-end on an Ubuntu 25.10 Fly machine via a build mirroring
CI exactly: ubuntu:24.04 docker, bun 1.3.11, node 22.13.0, full install
script. host-service comes up, registers with the relay, host appears
online to the cloud API.
Note: linux-arm64 doesn't ship @anush008/tokenizers (no published
linux-arm64-gnu binding). fastembed-dependent code paths will fail
there at runtime; non-fastembed paths still work.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBuild configuration for CLI and host-service packages is updated to explicitly manage native addon dependencies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 6/8 reviews remaining, refill in 9 minutes and 57 seconds.Comment |
Greptile SummaryThis PR fixes a crash-on-start regression in the published linux-x64 CLI tarball by externalizing Confidence Score: 5/5Safe to merge — the fix follows the established native-addon externalization pattern exactly, is verified end-to-end, and all remaining findings are P2 style/documentation notes. Both changed files apply a well-understood pattern that already works for better-sqlite3/node-pty. The only open items are a stale JSDoc comment and an undocumented-but-correct libsql side-fix, neither of which affects runtime behavior. The PR author verified the fix on a fresh Ubuntu 25.10 install. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/host-service/build.ts | Adds onnxruntime-node, @anush008/tokenizers (all platform variants), and libsql to the Bun external list, preventing the bundler from statically inlining their native-loader requires. Stale JSDoc comment still lists only the original two externals. |
| packages/cli/scripts/build-dist.ts | Adds onnxruntime-node and @anush008/tokenizers to NATIVE_PACKAGES (whole-package copy for all targets) and adds platform-specific @anush008/tokenizers-{darwin-universal,linux-x64-gnu} bindings to TARGET_NATIVE_PACKAGES. linux-arm64 intentionally omits the tokenizers binding (documented limitation). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bun run build:dist --target=linux-x64"] --> B["buildHostService()\nbun run build:host"]
B --> C{"Bun.build externals"}
C -->|externalized| D["better-sqlite3\nnode-pty\n@parcel/watcher\nlibsql ✨\nonnxruntime-node ✨\n@anush008/tokenizers ✨"]
C -->|bundled pure-JS| E["fastembed\n@mastra/fastembed"]
B --> F["dist/host-service.js\n(no content-hashed .node refs)"]
A --> G["copyNativePackages()"]
G --> H["NATIVE_PACKAGES\nbetter-sqlite3, node-pty\n@parcel/watcher, libsql\nonnxruntime-node ✨\n@anush008/tokenizers ✨"]
G --> I["TARGET_NATIVE_PACKAGES linux-x64"]
I --> J["@libsql/linux-x64-gnu\n@parcel/watcher-linux-x64-glibc\n@anush008/tokenizers-linux-x64-gnu ✨"]
H --> K["lib/node_modules/"]
J --> K
F --> L["lib/host-service.js"]
K --> M["superset-linux-x64.tar.gz"]
L --> M
Comments Outside Diff (1)
-
packages/host-service/build.ts, line 1-6 (link)Stale top-of-file comment
The JSDoc still says "Native addons (better-sqlite3, node-pty) are marked external" but the list now covers six packages. A quick update prevents future contributors from being confused about which deps are externalized.
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/build.ts Line: 1-6 Comment: **Stale top-of-file comment** The JSDoc still says "Native addons (better-sqlite3, node-pty) are marked external" but the list now covers six packages. A quick update prevents future contributors from being confused about which deps are externalized. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/build.ts:1-6
**Stale top-of-file comment**
The JSDoc still says "Native addons (better-sqlite3, node-pty) are marked external" but the list now covers six packages. A quick update prevents future contributors from being confused about which deps are externalized.
```suggestion
/**
* Bundles the host-service entry point into a single JS file that can be
* executed by a standalone Node.js runtime. Native addons (better-sqlite3,
* node-pty, @parcel/watcher, libsql, onnxruntime-node, @anush008/tokenizers)
* are marked external and must be resolved at runtime from
* lib/node_modules/ in the distribution bundle.
*/
```
### Issue 2 of 2
packages/host-service/build.ts:24
**`libsql` externalization fix is undocumented**
`libsql` was already in `NATIVE_PACKAGES` (and therefore copied into the tarball), but was not externalized in the Bun build before this PR. That means Bun previously statically processed `libsql`'s native-loader `require()` calls, producing the same class of content-hashed asset reference bug that this PR fixes for `onnxruntime-node`/`@anush008/tokenizers`. Adding it to `external` here is the right fix, but it isn't mentioned in the PR description — worth calling out so reviewers know this is intentional and that any `libsql` crash on a prior release would also be resolved by this change.
Reviews (1): Last reviewed commit: "fix(cli): ship onnxruntime-node + @anush..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "better-sqlite3", | ||
| "node-pty", | ||
| "@parcel/watcher", | ||
| "libsql", |
There was a problem hiding this comment.
libsql externalization fix is undocumented
libsql was already in NATIVE_PACKAGES (and therefore copied into the tarball), but was not externalized in the Bun build before this PR. That means Bun previously statically processed libsql's native-loader require() calls, producing the same class of content-hashed asset reference bug that this PR fixes for onnxruntime-node/@anush008/tokenizers. Adding it to external here is the right fix, but it isn't mentioned in the PR description — worth calling out so reviewers know this is intentional and that any libsql crash on a prior release would also be resolved by this change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/build.ts
Line: 24
Comment:
**`libsql` externalization fix is undocumented**
`libsql` was already in `NATIVE_PACKAGES` (and therefore copied into the tarball), but was not externalized in the Bun build before this PR. That means Bun previously statically processed `libsql`'s native-loader `require()` calls, producing the same class of content-hashed asset reference bug that this PR fixes for `onnxruntime-node`/`@anush008/tokenizers`. Adding it to `external` here is the right fix, but it isn't mentioned in the PR description — worth calling out so reviewers know this is intentional and that any `libsql` crash on a prior release would also be resolved by this change.
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Adds a configuration test that asserts both build pipelines list @anush008/tokenizers-linux-arm64-gnu alongside the linux-x64 binding fixed in #3921. The reproduction fails today because the package is omitted from: - TARGET_NATIVE_PACKAGES["linux-arm64"] in packages/cli/scripts/build-dist.ts - the Bun.build external array in packages/host-service/build.ts Also wires up `bun test` for @superset/cli so the reproduction runs under turbo. A real fix is blocked upstream — @anush008/tokenizers@0.0.0 does not declare the arm64 variant in optionalDependencies, and @anush008/tokenizers-linux-arm64-gnu@0.0.0 is not published on npm. Refs #3951
…rball (#3960) The linux-arm64 CLI tarball crashed on `superset start` with `Cannot find module '@anush008/tokenizers-linux-arm64-gnu'`. #3921 added the binding for linux-x64 but missed arm64. Three layers had to line up: 1. The build script needed the binding in TARGET_NATIVE_PACKAGES and the host-service bundler needed it as an external. 2. `@anush008/tokenizers@0.0.0` (pinned via fastembed → ^0.0.0) lists no linux-arm64 variant in optionalDependencies, so even with (1) bun couldn't resolve it. 3. `@anush008/tokenizers-linux-arm64-gnu@0.0.0` doesn't exist on npm (only 0.5.0 and 0.6.0 are published). Pin the 0.6.0 binding directly as an optionalDependency on host-service so bun installs it on linux-arm64 and skips it elsewhere via os/cpu filtering. The 0.6.0 native binary is ABI-compatible with the 0.0.0 JS loader for the surface fastembed uses (Tokenizer.fromString, setTruncation, setPadding, AddedToken, async encode, getIds, getTokens). Verified end-to-end in linux/arm64 docker: `bun install --frozen` picks up the binding, `build:dist --target=linux-arm64` ships it in lib/node_modules, the bundled Node loads `require("@anush008/tokenizers")` cleanly, and `superset-host` boots past the previous MODULE_NOT_FOUND. Closes #3951.
Summary
The published v0.2.0 linux-x64 tarball crashes immediately on
superset start:Verified by
superset starton a fresh Ubuntu 25.10 install of the published tarball — host service exits before binding a port.Root cause
The host-service bundle (built via
bun run build:host) externalizesbetter-sqlite3,node-pty,@parcel/watcher, andlibsqlso their.nodeloaders resolve at runtime fromlib/node_modules/. But it was bundling@anush008/tokenizersandonnxruntime-node(pulled in transitively through@mastra/fastembed → fastembed), so Bun's bundler statically processed their native-loaderrequire()s and emitted content-hashed asset references — without the matching.nodefiles alongside the bundle.Fix
Same pattern that already works for the other native deps:
packages/host-service/build.ts— addonnxruntime-nodeand@anush008/tokenizers(+ platform variants) to theexternallist.fastembedand@mastra/fastembedstay bundled (pure JS, no.nodefiles of their own).packages/cli/scripts/build-dist.ts— addonnxruntime-nodeand@anush008/tokenizerstoNATIVE_PACKAGESso they're copied whole intolib/node_modules/. Add@anush008/tokenizers-{darwin-universal,linux-x64-gnu}toTARGET_NATIVE_PACKAGESso each target tarball ships only the binding it needs.Verification
Built locally inside a docker container that mirrors CI's
ubuntu-latestrunner exactly (ubuntu:24.04base, bun 1.3.11, node 22.13.0,bun install --frozen --ignore-scripts→npx node-gyp rebuildfor node-pty →npm rebuild better-sqlite3 @parcel/watcher→bun run build:dist --target=linux-x64), pushed to a fresh sprite Ubuntu 25.10 box, ransuperset start:mcp__superset__hosts_listconfirms the host now reportsonline: truefrom the cloud's perspective. CI's existing smoke test inbuild-cli.ymlwould also catch a regression here:Worth extending that loop to include
onnxruntime-nodeand@anush008/tokenizersso future drift gets caught at build time. (Not in this PR — separate concern, easy follow-up.)Caveats
linux-arm64doesn't ship@anush008/tokenizers— no publishedlinux-arm64-gnubinding from the upstream package. Code paths that callfastembedwill fail at runtime on linux-arm64; non-fastembed paths still work. Adding arm64 support requires either upstream binaries or building from source.host-service/build.tsis only invoked by the CLI'sbuild-dist.ts(verified viagrep). Desktop bundles host-service through electron-vite + electron-builder, separate path.Test plan
build-cli.ymlproduces a green linux-x64 artifactsuperset start— expect the host service to come up and register with the relaySummary by CodeRabbit