fix(windows): bundle the LLVM runtime with Perry - #8018
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughWindows release and npm packaging now validate and bundle ChangesWindows LLVM packaging
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change bundles the required LLVM runtime with Windows builds and validates release and npm staging behavior; no actionable merge-blocking risk remains beyond normal checks. Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow as release-packages.yml
participant LLVMReadobj as llvm-readobj.exe
participant ReleaseStaging as Windows release staging
ReleaseWorkflow->>ReleaseWorkflow: Check perry.exe, llvm-readobj.exe, and LLVM-C.dll
ReleaseWorkflow->>LLVMReadobj: Inspect perry.exe imports
LLVMReadobj-->>ReleaseWorkflow: Report LLVM-C.dll import
ReleaseWorkflow->>ReleaseStaging: Stage perry.exe and LLVM-C.dll
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 |
proggeramlug
left a comment
There was a problem hiding this comment.
Audited exact commit 17b3726. The DLL import validation, release copy, npm staging refusal, and local copy/refusal regression are sound. One blocker remains in .github/workflows/gc-native-roots.yml: this PR changes the Windows status to FIXED and says the tar failure was fixed by --force-local, then says all four arms should pass. #8028 documents and changes the opposite: the runner uses bsdtar, --force-local was ineffective, and the Windows arm still fails until the path is normalized. Please make this status text accurate against the intended combined state (cygpath fix, with green status only after measured evidence). I have not merged this head.
|
Addressed in d0829e5. The workflow status now says Windows is only partially fixed, records that |
proggeramlug
left a comment
There was a problem hiding this comment.
Re-audited exact head d0829e5 after #8028 and #8027 landed. The functional packaging change is sound: release staging proves perry.exe imports LLVM-C.dll before copying it, npm staging requires/copies the DLL beside the executable, and the positive plus missing-DLL refusal regression passes locally. One blocker remains in the operational status comment: the merged result now says windows-latest is "pending #8028" and waits "until that fix is merged," but #8028 is already on main. Please update the combined-state wording to say the cygpath/native-env fix has landed while the arm remains unproven until a complete measured run. That preserves the important no-green-by-comment rule without leaving a false pending dependency. I have not merged this exact head. No version bump needed.
|
Addressed the remaining combined-state wording blocker in |
proggeramlug
left a comment
There was a problem hiding this comment.
Re-audited exact head 9dedfab. The prior operational-comment blocker is fixed: it now records both landed fixes while keeping the Windows arm unproven until a measured complete run. Release staging validates the LLVM-C.dll import before copying the runtime, npm staging fails closed when the DLL is absent, and the positive plus refusal regression passes locally. Merge-tree and diff checks are clean. No version bump.
Summary
LLVM-C.dllbesideperry.exein the Windows release zip@perryts/perry-win32-x64gc-native-rootsstatus after fix: restore CI portability and GC correctness #8017's dynamic-link fixperry.exeon currentmainlinks successfully after #8017, but importsLLVM-C.dll. The release workflow did not package that DLL, so an extracted release exited with Windows loader error0xC0000135unless LLVM happened to be installed globally. This completes the #7985 fix without restoring the incompatible static/MT+ rpmalloc link.Validation
cargo build --profile perry-dev -p perryon Windows with the official LLVM 22.1.8 archiveLLVM-C.dllandVCRUNTIME140.dllperry.exe+LLVM-C.dllbundle runs withC:\llvm\binremoved fromPATHtests/test_stage_npm_windows_llvm.shpasses its copy and missing-DLL refusal armsgit diff --checkNo version bump.
Closes #7985
Summary by CodeRabbit
Bug Fixes
LLVM-C.dllalongsideperry.exe.Tests