-
Notifications
You must be signed in to change notification settings - Fork 75
feat: fallback arm64 to x64 architecture for win32 platform #131
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
|
probably should throw warning letting user know this is emulating the x64 version with microsofts emulator and not actually using a native arm build, so as result it may be buggy. for example use the https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-a-warning-message method so it shows up in the status page where they will see if it the workflow failed. |
good idea 👍 |
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.
Pull Request Overview
Adds a temporary workaround for missing ARM64 Windows builds of Bun by falling back to x64 and updates the CI matrix to include an ARM Windows runner.
- Introduce
getEffectiveArchto maparm64→x64on Windows and emit a warning. - Update
getDownloadUrlto usegetEffectiveArch. - Expand GitHub Actions matrix with
windows-11-arm.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/action.ts | Add getEffectiveArch and integrate it into download URL encoding |
| .github/workflows/test.yml | Add windows-11-arm to the list of test runners |
Comments suppressed due to low confidence (5)
.github/workflows/test.yml:53
- The
windows-11-armrunner label is not a standard GitHub-hosted runner and will cause CI failures. Replace it with a valid ARM runner label or use a self-hosted runner.
- windows-11-arm
.github/workflows/test.yml:92
- Duplicate invalid runner label here; CI will not recognize
windows-11-arm. Consolidate or correct all occurrences.
- windows-11-arm
.github/workflows/test.yml:159
- This step also uses the non-existent
windows-11-armrunner. Please ensure all matrix entries use valid runner names.
- windows-11-arm
src/action.ts:158
- There are no existing unit tests covering the new
getEffectiveArchfallback behavior. Consider adding tests to verify Windows ARM64 is correctly remapped to x64 and the warning is emitted.
function getEffectiveArch(os: string, arch: string): string {
src/action.ts:158
- [nitpick] Consider adding a JSDoc comment above
getEffectiveArchto explain its purpose and parameters for future maintainers.
function getEffectiveArch(os: string, arch: string): string {
|
blocked until the bug in bun's download api is fixed |
|
@xhyrom is this till blocked ? can it be re-triggered ? thanks ! |
|
blocked by #93 |
|
Warning Rate limit exceeded@xhyrom has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Windows ARM support by implementing architecture detection that falls back Windows ARM to x64, introduces AVX2 flag handling for Windows ARM, extends test matrix to include windows-11-arm, and adds corresponding unit tests. Changes
Pre-merge checks✅ Passed checks (4 passed)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Add unit tests for getAvx2 function Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> * Consolidate duplicate test assertions in getAvx2 tests Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> * Fix whitespace formatting in test file Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com>
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.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/utils.ts (1)
46-46: Check for both "windows" and "win32" OS identifiers.The condition only checks for
os === "windows", but users might explicitly provideos: "win32"in action inputs (matching Node.js'sprocess.platformvalue). Whenosis explicitly provided, it bypasses thegetPlatform()normalization that converts "win32" to "windows", causing the fallback logic to fail.The same issue exists in
getAvx2at line 65.🔎 Recommended fix to handle both identifiers
- if (os === "windows" && (arch === "aarch64" || arch === "arm64")) { + if ((os === "windows" || os === "win32") && (arch === "aarch64" || arch === "arm64")) {Apply the same fix to
getAvx2at line 65:- if (os === "windows" && (arch === "aarch64" || arch === "arm64")) { + if ((os === "windows" || os === "win32") && (arch === "aarch64" || arch === "arm64")) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
.github/workflows/test.ymlsrc/download-url.tssrc/utils.tstests/utils.spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/utils.spec.ts (1)
src/utils.ts (1)
getAvx2(63-70)
src/download-url.ts (1)
src/utils.ts (3)
getArchitecture(45-61)getPlatform(38-43)getAvx2(63-70)
⏰ 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: setup-bun from (windows-latest, package.json (packageManager yarn@bun@1.1.0))
🔇 Additional comments (3)
.github/workflows/test.yml (1)
53-53: LGTM! Appropriate test coverage for Windows ARM64 fallback.Adding
windows-11-armto the test matrices forsetup-bunandsetup-bun-from-fileworkflows ensures the new fallback logic is validated on the target platform. The exclusion of this OS fromsetup-bun-download-urlis appropriate since that workflow manually constructs download URLs without the architecture fallback.Also applies to: 102-102
src/download-url.ts (1)
53-60: LGTM! Previous review issues have been addressed.The architecture resolution correctly:
- Calls
getArchitecture()unconditionally, ensuring the Windows ARM64 fallback applies even whenarchis explicitly provided- Uses
getPlatform()to normalizeprocess.platformfrom "win32" to "windows"- Passes the original (untransformed) arch value to
getAvx2(), which is correct since it needs to detect Windows ARM64 before the transformation to x64tests/utils.spec.ts (1)
1-49: LGTM! Comprehensive test coverage for getAvx2.The tests thoroughly validate the
getAvx2function across multiple dimensions:
- Windows ARM64/aarch64 behavior (returns false regardless of avx2 input)
- Explicit avx2 values on various platforms
- Default behavior (returns true) for non-Windows-ARM64 configurations
The test structure is clear, and all code paths are covered.
…llback logic (#153) * Initial plan * Add unit tests for getArchitecture function Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> * Remove unused import in utils.spec.ts Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Merge pr-131 and resolve test file conflicts Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xhyrom <56601352+xhyrom@users.noreply.github.com> Co-authored-by: Jozef Steinhübl <generalkubo@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
closes #130