-
Notifications
You must be signed in to change notification settings - Fork 49
fix: Windows compatibility - bun as optional dep, cross-env for examples #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
commit: |
|
@claude review this PR throroughly, highlight any potential native compatibility issue, security considerations, etc. |
| "@types/react": "^19.2.2", | ||
| "@types/react-dom": "^19.2.2", | ||
| "@vitejs/plugin-react": "^4.3.4", | ||
| "bun": "^1.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency causing a problem on Windows?
In 9f8d00e, I explicitly added bun as a dependency to the examples in order to improve on-ramping (users only have to run npm install).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I see the postinstall script added to package.json, but I'm still unclear on the problem that this dependency is causing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bun npm package was removed from examples' devDependencies (in the parent branch #144) because it was causing npm install to fail on Windows ARM64 - there's no native ARM64 binary for Bun on Windows yet, so the package's postinstall script throws "Unsupported platform: win32 arm64".
This PR takes a different approach: instead of relying on the bun npm package in each workspace, we add Bun's platform-specific packages (@oven/bun-darwin-aarch64, @oven/bun-linux-x64, etc.) as optionalDependencies in the root package.json. This has several benefits:
- Windows ARM64 support: Our
setup-bun.mjsscript handles this by downloading the x64-baseline binary, which runs via emulation - Single source of truth: Bun version is managed in one place (root package.json)
- Works with
npm install: Users still just runnpm install- the prepare script sets up bun automatically
The user experience is the same (or better) - npm install just works on all platforms including Windows ARM64.
jonathanhefner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that the setup-bun.mjs and run-bun.mjs scripts are necessary, but if it works, it works.
Fixes #142 - Windows PowerShell install bug Changes: - Replace single quotes with escaped double quotes in generate:schemas (cmd.exe passes single quotes literally, breaking prettier glob matching) - Simplify prettier scripts: remove redundant --ignore-path flags (prettier auto-loads .gitignore and .prettierignore by default) - Remove $(pwd) from prettier scripts (bash-only syntax) - Add cross-platform CI matrix: Linux x64, Windows x64, macOS ARM64, macOS x64 - Use shell: bash for grep commands in CI (not available in Windows cmd.exe) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The bun npm package tries to install platform-specific binaries which fails on Windows CI. Since bun is installed via oven-sh/setup-bun action in CI (and developers install it globally), we don't need the npm package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The examples use `INPUT=mcp-app.html vite build` which is bash syntax for setting environment variables. This doesn't work on Windows cmd.exe. Since examples aren't published to npm, we can skip building them on Windows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Git on Windows auto-converts line endings to CRLF, causing prettier to report formatting issues. Force LF for all text files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This verifies that `npm install git+https://...` works on all platforms, which triggers the prepare script and requires bun to be available. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Added ubuntu-24.04-arm (Linux ARM64) - Added windows-11-arm (Windows ARM64) Note: WSL is not available on GitHub-hosted runners (requires nested virtualization which isn't supported). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The oven-sh/setup-bun action fails with 404 when trying to download Bun for Windows ARM64. Re-enable when Bun adds proper support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Uses Vampire/setup-wsl action to install Ubuntu-24.04 in WSL and run the full build/test cycle. This tests the Linux-in-Windows scenario that was reported in issue #142. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The pkg-pr-new publish workflow was missing bun setup, causing "bun: not found" errors during npm ci (which triggers prepare script). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Windows ARM64 can run x64 binaries via emulation. Use bun-download-url to force x64-baseline build since native ARM64 Bun isn't available yet. Reference: oven-sh/bun#9824 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The macos-13 based runner images are now retired. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Documents that Bun is required for development, with platform-specific install commands in collapsible sections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add @oven/bun-* platform packages as optionalDependencies - Create postinstall script to set up bun binary from optional deps - Handle Windows ARM64 by downloading x64-baseline for emulation - Remove setup-bun from CI workflows (npm install handles it now) - Remove manual Bun install instructions from docs This alternative approach eliminates the need for global Bun installation or setup-bun action in CI. npm install automatically installs the platform-appropriate bun binary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add cross-env to devDependencies - Update all example package.json scripts to use cross-env for INPUT and NODE_ENV - Enable examples:build on Windows in CI (no longer Unix-only) - Add examples:build to WSL CI job 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Address github-code-quality bot comments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
During npm install's prepare hook, node_modules/.bin is not in PATH, so calling 'bun' directly fails. The new run-bun.mjs wrapper finds bun at node_modules/.bin/bun and runs it with the correct path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
395bdeb to
69b8eba
Compare
- Add setup-bun action to build and test-git-install jobs - Disable Windows ARM64 (Bun doesn't support this platform) - Add bun installation to WSL build 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Remove 'bun' from devDependencies (its postinstall fails on Windows ARM64) - Add libc detection (glibc vs musl) to setup-bun.mjs - Only try compatible binaries based on detected libc - Re-enable Windows ARM64 in CI (now uses x64 emulation fallback) - Remove setup-bun action from CI (no longer needed) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Fixes #142 - Windows PowerShell install failure.
This PR supersedes #144, taking a more comprehensive approach:
cross-envChanges
Bun setup (no global install needed):
@oven/bun-*platform-specific packages asoptionalDependenciesscripts/setup-bun.mjsto set up bun binary from optional depssetup-bunaction from all CI workflowsCross-platform fixes:
cross-envfor environment variables in example build scriptsINPUT=...andNODE_ENV=...syntaxexamples:buildon all platforms including WindowsCI improvements:
test-git-installverifies package works when installed from gitHow Bun setup works
npm installautomatically installs the platform-appropriate@oven/bun-*packagepreparescript runssetup-bun.mjswhich finds the binary and links it tonode_modules/.bin/bunDeveloper experience
npm installsetup-bunactionnpm installTest plan
npm installworks in fresh environmentnpm run buildandnpm run examples:buildwork on all platformsnpm testpassesCloses #144
🤖 Generated with Claude Code