refactor: Abstract out FS access from icp-project - #773
Open
adamspofford-dfinity wants to merge 8 commits into
Open
refactor: Abstract out FS access from icp-project#773adamspofford-dfinity wants to merge 8 commits into
adamspofford-dfinity wants to merge 8 commits into
Conversation
Consolidating a manifest reads the manifests it points at, the files its arguments and environment variables come from, and the directories its globs expand over; building reads back the module a build step produced. All of that went straight to `std::fs`, which a project loaded from somewhere other than a filesystem could never do. `files::FileSystem` is that surface now, with `HostFileSystem` behind a new default-on `host` feature. `Host` carries it, `ProjectLoadImpl` and `Builder` hold it, and `manifest::load_manifest_from_path`, `project::consolidate_manifest` and `operations::bundle::create_bundle` take it. The project-local `.icp` stores keep the traits they already had; only their implementations move behind `host`. `glob::glob` had to go: it walks the real filesystem itself, so no seam could stand in front of it. `files::expand_glob` matches a component at a time over `FileSystem::read_dir` instead, including the `**` the manifest reference documents. It also sorts each directory's entries, which the old code's own comment noted it could not do — so a bundle's canister ordering no longer depends on the order the filesystem happened to hand them back. Eight tests cover it, `**` included; the pattern it replaced had none. Three trait errors could no longer name their own source trees, since the implementation now lives on the far side of a feature gate. Each carries its cause opaquely, but the contextual fields stay — which environment's id store, which canister's artifact — so nothing the user reads is lost. `canonicalize` answers with `Option`, matching the `camino` method it replaces, so `BundleError::CanonicalizePath` no longer carries an `io::Error` it could not have. `cargo check -p icp-project --no-default-features` now passes: nothing outside the gate needs the host. That is not yet the wasm gate — every dependency is still non-optional and still linked, which is what the last stage is for — but it is the boundary the feature claims. Still host-shaped inside `icp-project`, and named here so the last stage has the list: `operations::build` makes a temp directory to build into, `ArchiveWriter::dir` uses `tar`'s own directory walk to keep symlinks as symlinks, `create.rs` draws on `rand`, and the subprocess and wasmtime step runners are untouched.
`FsError` rendered its boxed cause with `#[snafu(display("{source}"))]`,
which makes the cause both the wrapper's own message and its reported
source, so it prints twice in every chain it reaches. `#[snafu(transparent)]`
keeps the message and drops the wrapper from the chain.
Matches the seam errors in `network`, `canister::wasm` and
`canister::recipe`.
`expand_glob` matched every component by listing its parent and filtering
the names, and a listing never yields `..`, so any pattern containing one
matched nothing at all — silently. A monorepo whose root manifest reaches
a sibling directory (`canisters: [../shared/*]`) lost every canister it
names. An absolute pattern fared no better: splitting on `/` left a
leading empty component that was skipped, so `/opt/shared/*` walked from
the project directory instead of from the root.
Both are components that cannot match anything, so neither belongs in the
matching loop. Walking `Utf8Path::components()` instead of `split('/')`
names them: `ParentDir` is appended the way `join` appends it (and still
has to name a directory that is there), and `Prefix`/`RootDir` are pushed,
which is what discards the base — by the same rule that joining an
absolute path onto another discards the other.
That is the rule the non-glob branch of `build_manifest_canisters` has
always followed, since it is just `pdir.join(pattern)`. The two branches
now agree: a pattern with no metacharacters names the same path either
way.
Components also carry the platform's separators, so a `\`-spelled pattern
splits on Windows and stays literal elsewhere, matching what `glob::glob`
did with one.
`--no-default-features --features test-util` is the configuration the feature exists for — a build without the machine underneath it, with this crate's mocks still exposed so downstream tests can stand on the same seams — and it did not compile. `Host::mocked()` reached for `HostFileSystem`, which the `host` gate had just taken away, and the `store_id` mock used a `Mutex` whose import had moved behind the same gate. `files::UnimplementedMockFileSystem` is the seam's mock, alongside the ones `build`, `sync` and `wasm` already have; `Host::mocked()` takes it, so a mocked host is now mocks all the way down rather than the real filesystem in one field. Nothing that uses it touches files — every caller loads from `MockProjectLoader` — so the panic is the right answer if one ever starts. The `store_id` mock imports its own `Mutex` rather than borrowing the host implementation's, and the imports that only host code uses are gated, so the hostless build is warning-free too. Also drops a `#[cfg(feature = "host")]` that `artifact_name_overflow` carried twice.
`store_err` took a `&dyn Display` and rebuilt the cause as
`io::Error::other(e.to_string())`, which keeps the top frame's message and
drops everything under it. The store writes through `fs::write`, whose
error displays as "Filesystem operation failed at {path}" and carries the
real `io::Error` as its source — so a permission-denied or out-of-space
artifact write reported the path and never the reason.
It also printed that one surviving message twice: `StoreCause` displays as
its inner error but chains to that error's *source*, and for
`io::Error::other(String)` the source is a string error with the identical
message. The same duplication the seam errors in this stack have been
shedding.
`StoreCause::new` already carries any error whole, which is what
`store_id` does at each of its own call sites. A closure cannot be generic
over the error type, and the store fails in two of them — a lock and a
write — so this is a pair of free functions instead.
`prefixes_by_dir` is keyed by `FileSystem::canonicalize`, which for the host is `dunce::canonicalize` and strips the `\\?\` verbatim prefix. `Pruned::store_key` looked its directory up with `canonicalize_utf8()` — `std::fs::canonicalize`, which keeps that prefix. On Windows the two spellings never match, so the lookup always missed: `drops` answered false for every `<path>:<canister>` reference, and a bundle kept references to dependency canisters the selected environment leaves out. The extracted bundle rejects those at load, which is the failure this pruning exists to prevent. Both sides go through the seam now, so they spell a directory the same way by construction. `drops` is async in consequence, and the `retain` passes it fed cannot await, so `prune_environment` asks about every name the environment mentions up front and the passes became lookups. `mentioned_canisters` gathers those names and sits next to it: a name it fails to gather is a reference the bundle would keep.
`operations::build` made its build directory with `camino_tempfile` — a directory on this machine — handed the path to the build step, and then asked `files` whether the module was there and to read it back. The prebuilt step writes through `files` too, so today the two happen to agree; anything backing the seam with something other than this machine's filesystem would never see what the step wrote, and every build would end in `MissingWasmOutput`. `FileSystem::scratch_dir` hands out the directory instead, so the place the step writes to and the place the operation reads from are the same implementation's. The host's is a `tempfile` directory as before, removed when the returned `Scratch` drops. A script step still writes with the machine's own hands; that is the subprocess runner's host-shape, and it goes when the runner does.
`a_literal_component_needs_no_listing` described `glob::glob`'s optimization, not this code: every component is matched by listing its parent, which is what let a `..` slip through unmatched. The name now says what the test asserts, and it also asserts the other half — a literal component that names nothing yields nothing.
adamspofford-dfinity
added this pull request to stack #777
September 11, 2026 15:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack created with GitHub Stacks CLI • Give Feedback 💬