-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix/lancedb cross target #9033
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
base: main
Are you sure you want to change the base?
Fix/lancedb cross target #9033
Conversation
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
1 similar comment
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
|
✅ Review Complete Code Review for PR #9033Overall AssessmentThe changes look good and properly address the cross-target build issue. The refactoring improves clarity and correctness. Positive Changes
Issues and Suggestions1. Potential race condition with SQLite copying (Minor)
Suggestion: Review if the final 2. Missing package mapping warning could be clearer (Minor) Suggestion: If certain targets genuinely don't need LanceDB, document which ones and why. Otherwise, consider making this an error since a missing binary would likely cause runtime failures. 3. Consider testing with SKIP_INSTALLS (Testing)
The linked GitHub Actions run shows success, which is good, but explicit testing of these paths would increase confidence. No Action Required
|
|
Reviewed the PR changes. These are internal build script improvements that fix LanceDB binary installation for cross-platform builds. No documentation updates are needed as:
The PR is good to go from a documentation perspective. |
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.
No issues found across 1 file
Co-authored-by: nate <[email protected]>
|
Fixed prettier formatting issue in |
Validated with codex that the dry run publish passed, and that all artifacts had the binary in the right location
So every artifact now carries the native LanceDB binary for its target platform, and the macOS x64 package finally includes the x64 .node file.
Summary by cubic
Fix LanceDB binary installation for cross-target builds with explicit target-to-package mapping and ensure binaries are copied into the packaged output. Also only download sqlite when SKIP_INSTALLS is false and clean up temp directories.
Written for commit a8b5953. Summary will update automatically on new commits.