Skip to content

Migrate Tar-RO file system to new backend#1040

Open
jaybosamiya-ms wants to merge 7 commits into
mainfrom
jayb/tar-ro-migration
Open

Migrate Tar-RO file system to new backend#1040
jaybosamiya-ms wants to merge 7 commits into
mainfrom
jayb/tar-ro-migration

Conversation

@jaybosamiya-ms

Copy link
Copy Markdown
Member

This PR switches our Tar-RO filesystem to the new core file system design (see #887). Concretely, it adds a TarRo backend, migrates all old usage of tar_ro::FileSystem to a resolver-backed one to use the new TarRo backend, and then removes the old tar_ro::FileSystem. It also makes minor tweaks to the Backend interface to correctly account for read-only file systems. Interestingly, this move to the backend-based handling also gives us a chance to clean up the hardcoded dir stuff inside the tar-ro to instead properly use inode numbering :)

@jaybosamiya-ms

jaybosamiya-ms commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

This PR is easiest to review one commit in it at a time, rather than the whole thing at once. Also, just noting that semver-checks should (expectedly) mention a warning about the Backend trait having been tweaked + also tar_ro::FileSystem will get marked as disappeared, both are expected.

@jaybosamiya-ms
jaybosamiya-ms marked this pull request as ready for review July 17, 2026 22:44
@github-actions

Copy link
Copy Markdown

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_missing.ron

Failed in:
  struct litebox::fs::tar_ro::FileSystem, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/dc7c54620637cc11794b15754a562f4727249aaf/litebox/src/fs/tar_ro.rs:62

--- failure trait_method_parameter_count_changed: pub trait method parameter count changed ---

Description:
A trait method now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-item-signature
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/trait_method_parameter_count_changed.ron

Failed in:
  Backend::owned_dir_at now takes 2 instead of 1 parameters, in file /home/runner/work/litebox/litebox/litebox/src/fs/backend.rs:64

@wdcui

wdcui commented Jul 17, 2026

Copy link
Copy Markdown
Member

GPT-5.6 found two potential issues. I will add my review soon.

  1. Explicit directories can overwrite directory nodes —  litebox/src/fs/tar_ro.rs:397 
    A  dir/  tar entry may replace the directory created for  dir/file , causing descendant lookups to fail with  ENOTDIR .
  2. Tar directory  .  and  ..  entries now have inode 0 —  litebox_shim_linux/src/lib.rs:347 
     Resolver::read_dir  supplies no inode metadata for synthesized dot entries, changing prior behavior and potentially causing consumers to ignore them.

@jaybosamiya-ms

jaybosamiya-ms commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Thanks Weidong, for those two:

  1. tar_no_std does not give us dir entries (see https://docs.rs/tar-no-std/latest/tar_no_std/struct.ArchiveEntry.html). This is not a concern.
  2. this is expected, I already have this documented in the test. it is a resolver-level concern that needs to be fixed up there:
    if entry.name != "." && entry.name != ".." {
    assert!(entry.ino_info.is_some(), "Inode info should be present");
    } else {
    // TODO(jayb): Re-enable this assertion once Composer handles `.` and `..` inode
    // information better.
    }

let tar_ro = litebox::fs::resolver::Resolver::new(
litebox,
litebox::fs::composer::Composer::builder()
.mount("/", |allocator| {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, should we handle the case where it's not mounted at "/"?

@wdcui wdcui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants