Skip to content

ci: avoid shellexpand in lance-io#5985

Closed
lance-community wants to merge 1 commit intomainfrom
codex/fix-ci-22272601908
Closed

ci: avoid shellexpand in lance-io#5985
lance-community wants to merge 1 commit intomainfrom
codex/fix-ci-22272601908

Conversation

@lance-community
Copy link
Copy Markdown
Contributor

Link to failing workflow run: https://github.com/lance-format/lance/actions/runs/22272601908/job/64429623103?pr=5980

Summary of failures:

  • linux-build failed compiling shellexpand 3.1.1 on nightly (llvm-cov) with E0308 mismatched types.

Fixes applied:

  • Replaced shellexpand tilde expansion in lance-io with a local helper using the home crate.
  • Dropped the shellexpand dependency and added home to workspace and lance-io dependencies.
  • Updated Cargo.lock accordingly.

@github-actions github-actions Bot added the ci Github Action or Test issues label Feb 23, 2026
@github-actions
Copy link
Copy Markdown
Contributor

PR Review: ci: avoid shellexpand in lance-io

Summary: This PR replaces the shellexpand crate with a local expand_tilde helper using the home crate to fix nightly compilation failures.

P1 - Missing Unit Tests for expand_tilde

The new expand_tilde function at rust/lance-io/src/object_store.rs:356 handles critical path expansion logic but lacks dedicated unit tests. While test_tilde_expansion provides integration coverage for ~/path, consider adding unit tests for edge cases:

  • ~ alone (just home dir)
  • ~\path (Windows separator)
  • Non-tilde paths (passthrough)
  • home_dir() returning None (fallback behavior)

This is particularly important since path handling is security-sensitive and the function has subtle logic around separator handling.

Minor Observation

The implementation correctly drops support for ~username expansion (resolving other users' home directories), which shellexpand may have supported. This appears intentional and acceptable since Lance likely never relied on that feature - the existing test only covers ~/path.


Overall the fix is reasonable. The implementation looks correct for the supported use cases (~ and ~/path).

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2026

Codecov Report

❌ Patch coverage is 73.68421% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-io/src/object_store.rs 73.68% 3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Xuanwo added a commit that referenced this pull request Feb 23, 2026
This PR will replace #5985
since we don't need a new dep

## Summary

- remove `shellexpand` from workspace and `lance-io` dependencies
- replace tilde expansion in `lance-io` with `std::env::home_dir`-based
logic
- keep support for `~` and `~/...` paths (plus `~\\...` on Windows)
- update lockfile to drop `shellexpand`

## Validation

- `cargo check -p lance-io`
- `cargo test -p lance-io test_tilde_expansion -- --nocapture`
@Xuanwo Xuanwo closed this Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Github Action or Test issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants