Skip to content

Conversation

@Patrick-Erichsen
Copy link
Collaborator

@Patrick-Erichsen Patrick-Erichsen commented Dec 5, 2025

Validated with codex that the dry run publish passed, and that all artifacts had the binary in the right location

  • To be thorough I unpacked every VSIX from run 19980053236:
    • linux-x64: contains out/node_modules/@lancedb/vectordb-linux-x64-gnu/index.node.
    • linux-arm64 and linux-armhf: both ship …/vectordb-linux-arm64-gnu/index.node (plus the x64 variant that already lived in node_modules prior to these changes).
    • darwin-x64: confirmed both vectordb-darwin-x64/index.node and the pre-existing arm64 directory.
    • darwin-arm64: contains only vectordb-darwin-arm64/index.node.
    • win32-x64: has vectordb-win32-x64-msvc/index.node.
    • win32-arm64: includes vectordb-win32-arm64-msvc/index.node (and the x64 package we drag along with the whole @lancedb folder).

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.

  • Bug Fixes
    • Replace ARM-only check with full target mapping (darwin, linux, win32; arm64 and x64).
    • Install LanceDB package if missing and copy it to out/node_modules; log when present or missing mapping.
    • Read build target from env and pass --target during packaging to keep the correct binary in the .vsix.
    • Only download sqlite binding when SKIP_INSTALLS is false; add clear log messages.
    • Remove temporary install directory with rimraf; warn on failure.

Written for commit a8b5953. Summary will update automatically on new commits.

@Patrick-Erichsen Patrick-Erichsen requested a review from a team as a code owner December 5, 2025 23:10
@Patrick-Erichsen Patrick-Erichsen requested review from RomneyDa and removed request for a team December 5, 2025 23:10
@continue
Copy link
Contributor

continue bot commented Dec 5, 2025

Keep this PR in a mergeable state →

Learn more

All Green is an AI agent that automatically:

✅ Addresses code review comments

✅ Fixes failing CI checks

✅ Resolves merge conflicts

1 similar comment
@continue-development-app
Copy link

Keep this PR in a mergeable state →

Learn more

All Green is an AI agent that automatically:

✅ Addresses code review comments

✅ Fixes failing CI checks

✅ Resolves merge conflicts

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 5, 2025
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

⚠️ PR Title Format

Your PR title doesn't follow the conventional commit format, but this won't block your PR from being merged. We recommend using this format for better project organization.

Expected Format:

<type>[optional scope]: <description>

Examples:

  • feat: add changelog generation support
  • fix: resolve login redirect issue
  • docs: update README with new instructions
  • chore: update dependencies

Valid Types:

feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert

This helps with:

  • 📝 Automatic changelog generation
  • 🚀 Automated semantic versioning
  • 📊 Better project history tracking

This is a non-blocking warning - your PR can still be merged without fixing this.

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

✅ Review Complete

Code Review for PR #9033

Overall Assessment

The changes look good and properly address the cross-target build issue. The refactoring improves clarity and correctness.

Positive Changes

  1. Complete target mapping - Now includes all 6 platforms (darwin/linux/win32 × arm64/x64) instead of just ARM targets
  2. Idempotent installation - Checks if package exists before installing
  3. Better logging - Clear messages for what's happening and why
  4. Separation of concerns - LanceDB installation is now independent of skipInstalls

Issues and Suggestions

1. Potential race condition with SQLite copying (Minor)
Line 321: The copySqlite call is now conditional on skipInstalls, but the comment on line 324 says "Copying sqlite node binding from core". This suggests copySqlite might be doing two things:

  • Downloading/installing (should respect skipInstalls)
  • Copying already-built bindings (should always happen)

Suggestion: Review if the final console.log on line 324 is still accurate, or if that copying step also needs to be conditional.

2. Missing package mapping warning could be clearer (Minor)
Line 316: The warning for missing package mapping might be unexpected. Consider if this should be an error instead, or if there are legitimate cases where a target won't have a LanceDB package.

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 new logic around skipInstalls and the idempotent installation should be tested with:

  • SKIP_INSTALLS=true
  • Package already present
  • Package not present

The linked GitHub Actions run shows success, which is good, but explicit testing of these paths would increase confidence.

No Action Required

  • The removal of isArmTarget is correct
  • The package name parsing and path construction looks solid
  • The async/await handling is appropriate

@continue
Copy link
Contributor

continue bot commented Dec 5, 2025

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:

  • This is an internal packaging process change
  • No user-facing features or workflows are affected
  • CONTRIBUTING.md setup steps remain the same
  • The changes are implementation details of the build system

The PR is good to go from a documentation perspective.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a 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

@continue
Copy link
Contributor

continue bot commented Dec 5, 2025

Fixed prettier formatting issue in prepackage.js. The CI failure was due to a console.warn line exceeding the line length limit. Pushed a fix that wraps the long line.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants