From a2e2600df8d1bbdcf690c737b239b743b649babc Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Wed, 9 Sep 2026 10:04:18 -0700 Subject: [PATCH 01/12] refactor: reach the filesystem through a seam, and gate the host half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/icp-app/src/context/init.rs | 7 +- crates/icp-cli/src/commands/build.rs | 1 + crates/icp-cli/src/commands/project/bundle.rs | 1 + crates/icp-project/Cargo.toml | 5 + crates/icp-project/src/canister/build/mod.rs | 16 +- .../src/canister/build/prebuilt.rs | 10 +- crates/icp-project/src/files.rs | 323 ++++++++++++++++++ crates/icp-project/src/fs/mod.rs | 15 + crates/icp-project/src/host.rs | 4 + crates/icp-project/src/lib.rs | 10 +- crates/icp-project/src/manifest/mod.rs | 26 +- crates/icp-project/src/operations/build.rs | 12 +- crates/icp-project/src/operations/bundle.rs | 172 ++++++---- crates/icp-project/src/operations/deploy.rs | 10 +- crates/icp-project/src/project.rs | 315 +++++++++-------- crates/icp-project/src/store_artifact.rs | 68 ++-- crates/icp-project/src/store_id.rs | 104 ++++-- 17 files changed, 827 insertions(+), 272 deletions(-) create mode 100644 crates/icp-project/src/files.rs diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index bf02207dd..3f21a2a9b 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -106,8 +106,11 @@ pub fn initialize( pkg_cache, }); + // The project's files come from this machine. + let files = Arc::new(icp_project::files::HostFileSystem); + // Canister builder - let builder = Arc::new(Builder::new(wasm.clone())); + let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); // Canister syncer let syncer = Arc::new(Syncer::host(wasm.clone())); @@ -116,6 +119,7 @@ pub fn initialize( let pload = ProjectLoadImpl { project_root_locate: project_root_locate.clone(), recipe, + files: files.clone(), }; let pload = Lazy::new(pload); @@ -153,6 +157,7 @@ pub fn initialize( Ok(Context { host: Host { project: pload, + files, ids, artifacts, builder, diff --git a/crates/icp-cli/src/commands/build.rs b/crates/icp-cli/src/commands/build.rs index fe92be293..e6dcfc876 100644 --- a/crates/icp-cli/src/commands/build.rs +++ b/crates/icp-cli/src/commands/build.rs @@ -65,6 +65,7 @@ pub(crate) async fn exec(ctx: &Context, args: &BuildArgs) -> Result<(), anyhow:: environment_selection.name(), ctx.host.builder.clone(), ctx.host.artifacts.clone(), + ctx.host.files.as_ref(), reporter, ) .await diff --git a/crates/icp-cli/src/commands/project/bundle.rs b/crates/icp-cli/src/commands/project/bundle.rs index fd87932dd..92ca2c29d 100644 --- a/crates/icp-cli/src/commands/project/bundle.rs +++ b/crates/icp-cli/src/commands/project/bundle.rs @@ -53,6 +53,7 @@ pub(crate) async fn exec(ctx: &Context, args: &BundleArgs) -> Result<(), anyhow: rendered(ctx.debug, async |reporter| { create_bundle( + ctx.host.files.as_ref(), &project.dir, canisters, &selected, diff --git a/crates/icp-project/Cargo.toml b/crates/icp-project/Cargo.toml index 491ca2ab0..3d8d061a9 100644 --- a/crates/icp-project/Cargo.toml +++ b/crates/icp-project/Cargo.toml @@ -6,6 +6,11 @@ license = { workspace = true } publish.workspace = true [features] +default = ["host"] +# Implementations of this crate's seams that use the machine it is running on: +# the filesystem, the project-local `.icp` stores, subprocesses. Turned off for +# a build that has to run somewhere without them, such as inside a canister. +host = [] # Exposes this crate's mocks and fixtures so downstream crates can test against # the same seams. Off in a normal build, so none of it ships. test-util = [] diff --git a/crates/icp-project/src/canister/build/mod.rs b/crates/icp-project/src/canister/build/mod.rs index 3aafef453..7cefb961a 100644 --- a/crates/icp-project/src/canister/build/mod.rs +++ b/crates/icp-project/src/canister/build/mod.rs @@ -40,11 +40,12 @@ pub trait Build: Sync + Send { /// a pre-built step by asking [`wasm::Fetch`] for the module. pub struct Builder { wasm: Arc, + files: Arc, } impl Builder { - pub fn new(wasm: Arc) -> Self { - Self { wasm } + pub fn new(wasm: Arc, files: Arc) -> Self { + Self { wasm, files } } } @@ -57,9 +58,14 @@ impl Build for Builder { reporter: &StepReporter, ) -> Result<(), BuildError> { match step { - BuildStep::Prebuilt(adapter) => { - Ok(prebuilt::build(adapter, params, reporter, self.wasm.as_ref()).await?) - } + BuildStep::Prebuilt(adapter) => Ok(prebuilt::build( + adapter, + params, + reporter, + self.wasm.as_ref(), + self.files.as_ref(), + ) + .await?), BuildStep::Script(adapter) => Ok(script::build(adapter, params, reporter).await?), } } diff --git a/crates/icp-project/src/canister/build/prebuilt.rs b/crates/icp-project/src/canister/build/prebuilt.rs index c2dbb22bf..749dacfa6 100644 --- a/crates/icp-project/src/canister/build/prebuilt.rs +++ b/crates/icp-project/src/canister/build/prebuilt.rs @@ -1,7 +1,7 @@ use icp_events::StepReporter; use snafu::prelude::*; -use crate::{canister::wasm, fs, manifest::adapter::prebuilt::Adapter}; +use crate::{canister::wasm, manifest::adapter::prebuilt::Adapter}; use super::Params; @@ -11,7 +11,7 @@ pub enum PrebuiltError { Wasm { source: wasm::FetchError }, #[snafu(display("failed to copy wasm to output file"))] - CopyFile { source: crate::fs::CopyError }, + CopyFile { source: crate::files::FsError }, } pub(super) async fn build( @@ -19,6 +19,7 @@ pub(super) async fn build( params: &Params, reporter: &StepReporter, wasm: &dyn wasm::Fetch, + files: &dyn crate::files::FileSystem, ) -> Result<(), PrebuiltError> { let src = wasm .wasm( @@ -30,7 +31,10 @@ pub(super) async fn build( .await?; reporter.info(format!("Writing WASM file: {}", params.output)); - fs::copy(&src, ¶ms.output).context(CopyFileSnafu)?; + files + .copy(&src, ¶ms.output) + .await + .context(CopyFileSnafu)?; Ok(()) } diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs new file mode 100644 index 000000000..71e800407 --- /dev/null +++ b/crates/icp-project/src/files.rs @@ -0,0 +1,323 @@ +//! Reading and writing the files a project is made of. +//! +//! Consolidating a manifest means reading the manifests it points at, the files +//! its arguments and environment variables come from, and the directories its +//! globs expand over. Building means reading back the module a build step +//! produced. None of that is necessarily a filesystem: the same project could +//! be described by blobs in a canister's stable memory. So it is asked for +//! through [`FileSystem`] rather than done here. +//! +//! [`fs`](crate::fs) is the host implementation's own vocabulary — thin +//! wrappers over `std::fs` whose errors carry the path. Project code should +//! not reach for it. + +use async_trait::async_trait; +use snafu::{ResultExt, Snafu}; + +use crate::prelude::*; + +/// A file operation failed. +/// +/// What backs the files is the implementation's business — a real filesystem, a +/// bundle being unpacked, stable memory — so the cause is carried whole and +/// displayed as itself. Implementations name the path in their own error, which +/// is what a reader needs to see. +#[derive(Debug, Snafu)] +#[snafu(display("{source}"))] +pub struct FsError { + pub source: Box, +} + +impl FsError { + /// Wraps an implementation's own error for the trait boundary. + pub fn new(source: impl std::error::Error + Send + Sync + 'static) -> Self { + Self { + source: Box::new(source), + } + } +} + +/// Where a project's files come from. +/// +/// The predicates (`exists`, `is_file`, `is_dir`) answer `false` on any error, +/// matching the `camino` inherent methods they replace: a caller asking whether +/// something is there has nothing useful to do with the difference between "no" +/// and "could not tell". [`canonicalize`](FileSystem::canonicalize) returns +/// `None` for the same reason, and callers treat that as "cannot establish +/// identity", which is the safe answer when the question is whether two paths +/// are the same file. +#[async_trait] +pub trait FileSystem: Send + Sync { + async fn read(&self, path: &Path) -> Result, FsError>; + + async fn read_to_string(&self, path: &Path) -> Result; + + async fn write(&self, path: &Path, contents: &[u8]) -> Result<(), FsError>; + + async fn create_dir_all(&self, path: &Path) -> Result<(), FsError>; + + async fn copy(&self, from: &Path, to: &Path) -> Result<(), FsError>; + + async fn exists(&self, path: &Path) -> bool; + + async fn is_file(&self, path: &Path) -> bool; + + async fn is_dir(&self, path: &Path) -> bool; + + /// Non-recursive listing. Entries come back as `path` joined with each + /// entry's name, so they are usable as-is. + async fn read_dir(&self, path: &Path) -> Result, FsError>; + + /// Resolve `..` and symlinks. `None` when the path does not resolve. + async fn canonicalize(&self, path: &Path) -> Option; +} + +#[cfg(feature = "host")] +/// The [`FileSystem`] backed by this machine's filesystem. +#[derive(Debug, Default, Clone, Copy)] +pub struct HostFileSystem; + +#[cfg(feature = "host")] +#[async_trait] +impl FileSystem for HostFileSystem { + async fn read(&self, path: &Path) -> Result, FsError> { + crate::fs::read(path).map_err(FsError::new) + } + + async fn read_to_string(&self, path: &Path) -> Result { + crate::fs::read_to_string(path).map_err(FsError::new) + } + + async fn write(&self, path: &Path, contents: &[u8]) -> Result<(), FsError> { + crate::fs::write(path, contents).map_err(FsError::new) + } + + async fn create_dir_all(&self, path: &Path) -> Result<(), FsError> { + crate::fs::create_dir_all(path).map_err(FsError::new) + } + + async fn copy(&self, from: &Path, to: &Path) -> Result<(), FsError> { + crate::fs::copy(from, to).map_err(FsError::new) + } + + async fn exists(&self, path: &Path) -> bool { + path.exists() + } + + async fn is_file(&self, path: &Path) -> bool { + path.is_file() + } + + async fn is_dir(&self, path: &Path) -> bool { + path.is_dir() + } + + async fn read_dir(&self, path: &Path) -> Result, FsError> { + crate::fs::read_dir(path).map_err(FsError::new) + } + + async fn canonicalize(&self, path: &Path) -> Option { + PathBuf::from_path_buf(dunce::canonicalize(path).ok()?).ok() + } +} + +/// A glob pattern could not be understood. +#[derive(Debug, Snafu)] +pub enum GlobError { + #[snafu(display("'{pattern}' is not a valid glob pattern"))] + Pattern { + source: glob::PatternError, + pattern: String, + }, + + #[snafu(display("failed to list '{path}' while expanding glob '{pattern}'"))] + List { + source: FsError, + path: PathBuf, + pattern: String, + }, +} + +/// Expand `pattern` against `files`, relative to `base`. +/// +/// The `glob` crate walks the real filesystem itself, so it is no use where the +/// files are not one. Matching happens a component at a time instead, with +/// `**` standing for any number of directories — the same shape `glob` +/// supports, and the same one the manifest reference documents. +/// +/// Only paths that exist are returned, and each directory's entries are listed +/// in sorted order, so the result is the same on every run. +pub async fn expand_glob( + files: &dyn FileSystem, + base: &Path, + pattern: &str, +) -> Result, GlobError> { + let mut frontier = vec![base.to_path_buf()]; + + for component in pattern.split('/') { + if component.is_empty() || component == "." { + continue; + } + + if component == "**" { + // `**` matches zero or more directories, so every reachable + // directory — including the ones already in hand — carries forward. + let mut reached = frontier.clone(); + let mut stack = frontier; + while let Some(dir) = stack.pop() { + for entry in list(files, &dir, pattern).await? { + if files.is_dir(&entry).await { + reached.push(entry.clone()); + stack.push(entry); + } + } + } + reached.sort(); + reached.dedup(); + frontier = reached; + continue; + } + + let matcher = glob::Pattern::new(component).context(PatternSnafu { + pattern: pattern.to_owned(), + })?; + + let mut next = Vec::new(); + for dir in &frontier { + for entry in list(files, dir, pattern).await? { + if entry.file_name().is_some_and(|name| matcher.matches(name)) { + next.push(entry); + } + } + } + next.sort(); + next.dedup(); + frontier = next; + } + + Ok(frontier) +} + +/// Entries of `dir`, or none when it cannot be listed because it is not a +/// directory. A glob reaching past a plain file matches nothing rather than +/// failing. +async fn list( + files: &dyn FileSystem, + dir: &Path, + pattern: &str, +) -> Result, GlobError> { + if !files.is_dir(dir).await { + return Ok(Vec::new()); + } + files.read_dir(dir).await.context(ListSnafu { + path: dir.to_path_buf(), + pattern: pattern.to_owned(), + }) +} + +#[cfg(all(test, feature = "host"))] +mod tests { + use super::*; + + /// Builds a tree of empty files, creating parents as needed, and returns its + /// root. + fn tree(paths: &[&str]) -> camino_tempfile::Utf8TempDir { + let dir = camino_tempfile::Utf8TempDir::new().expect("temp dir"); + for p in paths { + let full = dir.path().join(p); + std::fs::create_dir_all(full.parent().expect("has a parent")).expect("mkdir"); + std::fs::write(&full, b"").expect("write"); + } + dir + } + + async fn expand(root: &Path, pattern: &str) -> Vec { + expand_glob(&HostFileSystem, root, pattern) + .await + .expect("expand") + .into_iter() + .map(|p| { + p.strip_prefix(root) + .expect("under root") + .as_str() + .replace('\\', "/") + }) + .collect() + } + + #[tokio::test] + async fn a_single_star_matches_within_one_directory_only() { + let d = tree(&["canisters/a/canister.yaml", "canisters/b/canister.yaml"]); + assert_eq!( + expand(d.path(), "canisters/*").await, + ["canisters/a", "canisters/b"] + ); + } + + #[tokio::test] + async fn a_literal_component_needs_no_listing() { + let d = tree(&["canisters/a/canister.yaml"]); + assert_eq!( + expand(d.path(), "canisters/a/canister.yaml").await, + ["canisters/a/canister.yaml"] + ); + } + + /// `**` stands for zero or more directories, so a pattern that uses it also + /// matches at the depth where it stands for none. + #[tokio::test] + async fn a_double_star_matches_at_every_depth_including_zero() { + let d = tree(&[ + "services/one.yaml", + "services/a/two.yaml", + "services/a/b/three.yaml", + "elsewhere/four.yaml", + ]); + assert_eq!( + expand(d.path(), "services/**/*.yaml").await, + [ + "services/a/b/three.yaml", + "services/a/two.yaml", + "services/one.yaml", + ] + ); + } + + #[tokio::test] + async fn character_classes_and_question_marks_work() { + let d = tree(&["c/a1.yaml", "c/b2.yaml", "c/cc.yaml"]); + assert_eq!( + expand(d.path(), "c/?[0-9].yaml").await, + ["c/a1.yaml", "c/b2.yaml"] + ); + } + + /// Reaching through a plain file matches nothing, rather than failing: the + /// pattern simply describes no path here. + #[tokio::test] + async fn descending_through_a_file_matches_nothing() { + let d = tree(&["notadir"]); + assert!(expand(d.path(), "notadir/*").await.is_empty()); + } + + #[tokio::test] + async fn a_pattern_matching_nothing_yields_nothing() { + let d = tree(&["canisters/a/canister.yaml"]); + assert!(expand(d.path(), "services/*").await.is_empty()); + } + + #[tokio::test] + async fn results_are_sorted_so_a_run_is_reproducible() { + let d = tree(&["c/z/x", "c/a/x", "c/m/x"]); + assert_eq!(expand(d.path(), "c/*").await, ["c/a", "c/m", "c/z"]); + } + + #[tokio::test] + async fn a_malformed_pattern_is_reported_as_such() { + let d = tree(&["c/a"]); + let err = expand_glob(&HostFileSystem, d.path(), "c/[a-") + .await + .expect_err("unterminated class"); + assert!(matches!(err, GlobError::Pattern { .. })); + } +} diff --git a/crates/icp-project/src/fs/mod.rs b/crates/icp-project/src/fs/mod.rs index 6b16cfb2d..761aa078e 100644 --- a/crates/icp-project/src/fs/mod.rs +++ b/crates/icp-project/src/fs/mod.rs @@ -48,6 +48,21 @@ pub fn read_to_string(path: &Path) -> Result { std::fs::read_to_string(path).context(IoSnafu { path }) } +/// Non-recursive directory listing, as full paths. A name that is not UTF-8 +/// cannot be named by any manifest, so it is skipped rather than being an +/// error. +pub fn read_dir(path: &Path) -> Result, IoError> { + let mut out = Vec::new(); + for entry in std::fs::read_dir(path).context(IoSnafu { path })? { + let entry = entry.context(IoSnafu { path })?; + if let Ok(p) = PathBuf::from_path_buf(entry.path()) { + out.push(p); + } + } + out.sort(); + Ok(out) +} + pub fn remove_dir_all(path: &Path) -> Result<(), IoError> { std::fs::remove_dir_all(path).context(IoSnafu { path }) } diff --git a/crates/icp-project/src/host.rs b/crates/icp-project/src/host.rs index a8ac7633f..53255da21 100644 --- a/crates/icp-project/src/host.rs +++ b/crates/icp-project/src/host.rs @@ -56,6 +56,9 @@ pub struct Host { /// Project loader pub project: Arc, + /// Where the project's files come from + pub files: Arc, + /// Canister ID store for lookup and storage pub ids: Arc, @@ -107,6 +110,7 @@ impl Host { pub fn mocked() -> Self { Self { project: Arc::new(crate::MockProjectLoader::minimal()), + files: Arc::new(crate::files::HostFileSystem), ids: Arc::new(crate::store_id::mock::MockInMemoryIdStore::new()), artifacts: Arc::new(crate::store_artifact::MockInMemoryArtifactStore::new()), builder: Arc::new(crate::canister::build::UnimplementedMockBuilder), diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index cab2d97d7..200a44b1b 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -14,6 +14,7 @@ use candid_parser::parse_idl_args; use crate::{ canister::{Settings, recipe::Resolve}, + files::FileSystem, manifest::{ ArgsFormat, LoadManifestFromPathError, PROJECT_MANIFEST, ProjectRootLocate, ProjectRootLocateError, @@ -26,6 +27,8 @@ use crate::{ pub mod agent; pub mod canister; +pub mod files; +#[cfg(feature = "host")] pub mod fs; pub mod host; pub mod manifest; @@ -232,6 +235,7 @@ pub trait ProjectLoad: Sync + Send { pub struct ProjectLoadImpl { pub project_root_locate: Arc, pub recipe: Arc, + pub files: Arc, } /// Ensures the "operating on a workspace root above your sub-project" notice is @@ -275,14 +279,14 @@ impl ProjectLoad for ProjectLoadImpl { } // Load project manifest - let m = load_manifest_from_path(&pdir.join(PROJECT_MANIFEST)) + let m = load_manifest_from_path(self.files.as_ref(), &pdir.join(PROJECT_MANIFEST)) .await .context(ProjectManifestSnafu)?; debug!("Loaded project manifest: {m:#?}"); // Consolidate manifest into project - let p = project::consolidate_manifest(&pdir, self.recipe.as_ref(), &m) + let p = project::consolidate_manifest(self.files.as_ref(), &pdir, self.recipe.as_ref(), &m) .await .context(ProjectSnafu)?; @@ -769,6 +773,7 @@ mod tests { let loader = ProjectLoadImpl { project_root_locate: Arc::new(MockProjectRootLocate::new(project_dir.to_path_buf())), recipe: Arc::new(MockRecipeResolver), + files: Arc::new(crate::files::HostFileSystem), }; // Call load @@ -829,6 +834,7 @@ mod tests { let loader = ProjectLoadImpl { project_root_locate: Arc::new(MockProjectRootLocate::new(project_dir.to_path_buf())), recipe: Arc::new(MockRecipeResolver), + files: Arc::new(crate::files::HostFileSystem), }; // Call load diff --git a/crates/icp-project/src/manifest/mod.rs b/crates/icp-project/src/manifest/mod.rs index 10d54c27f..3172b0909 100644 --- a/crates/icp-project/src/manifest/mod.rs +++ b/crates/icp-project/src/manifest/mod.rs @@ -5,6 +5,8 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use snafu::prelude::*; +use crate::files::FileSystem; +#[cfg(feature = "host")] use crate::fs; use crate::prelude::*; @@ -127,6 +129,7 @@ pub trait ProjectRootLocate: Sync + Send { } /// Implementation of [`ProjectRootLocate`]. +#[cfg(feature = "host")] pub struct ProjectRootLocateImpl { /// Current directory to begin search from in case dir is unspecified. cwd: PathBuf, @@ -135,6 +138,7 @@ pub struct ProjectRootLocateImpl { dir: Option, } +#[cfg(feature = "host")] impl ProjectRootLocateImpl { /// Creates a new instance of `ProjectRootLocateImpl`. /// @@ -145,6 +149,7 @@ impl ProjectRootLocateImpl { } } +#[cfg(feature = "host")] /// The nearest directory at or above `start` that contains a project manifest. fn nearest_manifest_dir(start: &Path) -> Option { let mut dir = start.to_owned(); @@ -157,6 +162,7 @@ fn nearest_manifest_dir(start: &Path) -> Option { } /// The nearest directory *strictly above* `dir` that contains a project manifest. +#[cfg(feature = "host")] fn next_manifest_dir_above(dir: &Path) -> Option { let mut cur = dir.parent()?.to_owned(); loop { @@ -170,6 +176,7 @@ fn next_manifest_dir_above(dir: &Path) -> Option { /// Canonicalize a directory (resolving `..` and symlinks) into a UTF-8 path. /// Returns `None` if the path does not exist or is not valid UTF-8; callers /// treat that as "cannot establish identity", which is safe for resolution. +#[cfg(feature = "host")] fn canonicalize_dir(dir: &Path) -> Option { let canon = dunce::canonicalize(dir.as_std_path()).ok()?; PathBuf::try_from(canon).ok() @@ -179,6 +186,7 @@ fn canonicalize_dir(dir: &Path) -> Option { /// other field. Deliberately lenient: any read/parse failure yields no /// dependencies, so an unrelated or malformed ancestor manifest is treated as /// declaring nothing (it will not be adopted as a workspace root). +#[cfg(feature = "host")] fn read_dependency_paths(manifest_path: &Path) -> Vec { #[derive(Deserialize)] struct DepProbe { @@ -203,6 +211,7 @@ fn read_dependency_paths(manifest_path: &Path) -> Vec { /// transitively. Each `path:` is resolved relative to the manifest that /// declares it, then canonicalized so identity is independent of how the path /// is spelled (matches [`crate::project`] dependency de-duplication). +#[cfg(feature = "host")] fn transitive_dep_dirs(manifest_dir: &Path) -> HashSet { let mut out = HashSet::new(); let Some(start) = canonicalize_dir(manifest_dir) else { @@ -224,6 +233,7 @@ fn transitive_dep_dirs(manifest_dir: &Path) -> HashSet { out } +#[cfg(feature = "host")] impl ProjectRootLocate for ProjectRootLocateImpl { fn locate(&self) -> Result { // Start from the project the command is standing in. An explicit @@ -280,8 +290,11 @@ impl ProjectRootLocate for ProjectRootLocateImpl { #[derive(Debug, Snafu)] pub enum LoadManifestFromPathError { - #[snafu(display("failed to read manifest from path"))] - Read { source: fs::IoError }, + #[snafu(display("failed to read manifest at '{path}'"))] + Read { + source: crate::files::FsError, + path: PathBuf, + }, #[snafu(display("failed to parse manifest at '{path}'"))] Parse { @@ -291,11 +304,16 @@ pub enum LoadManifestFromPathError { } /// Loads a manifest of type `T` from the specified file path. -pub async fn load_manifest_from_path(path: &Path) -> Result +pub async fn load_manifest_from_path( + files: &dyn FileSystem, + path: &Path, +) -> Result where T: for<'de> Deserialize<'de>, { - let content = fs::read(path).context(ReadSnafu)?; + let content = files.read(path).await.context(ReadSnafu { + path: path.to_path_buf(), + })?; let m = serde_yaml::from_slice::(&content).context(ParseSnafu { path: path.to_path_buf(), })?; diff --git a/crates/icp-project/src/operations/build.rs b/crates/icp-project/src/operations/build.rs index 3baeeb8d3..cd85520ae 100644 --- a/crates/icp-project/src/operations/build.rs +++ b/crates/icp-project/src/operations/build.rs @@ -24,7 +24,7 @@ pub enum BuildOperationError { MissingWasmOutput, #[snafu(display("failed to read wasm output file"))] - ReadWasmOutput { source: crate::fs::IoError }, + ReadWasmOutput { source: crate::files::FsError }, #[snafu(display("failed to save wasm artifact"))] SaveWasmArtifact { @@ -45,6 +45,7 @@ pub async fn build( task: &TaskReporter, builder: Arc, artifacts: Arc, + files: &dyn crate::files::FileSystem, ) -> Result<(), BuildOperationError> { let build_dir = tempdir().context(TempDirSnafu)?; let wasm_output_path = build_dir.path().join("out.wasm"); @@ -73,11 +74,14 @@ pub async fn build( build_result?; } - if !wasm_output_path.exists() { + if !files.exists(&wasm_output_path).await { return MissingWasmOutputSnafu.fail(); } - let wasm = crate::fs::read(&wasm_output_path).context(ReadWasmOutputSnafu)?; + let wasm = files + .read(&wasm_output_path) + .await + .context(ReadWasmOutputSnafu)?; artifacts .save(&canister.name, &wasm) @@ -92,6 +96,7 @@ pub async fn build_many( environment: &str, builder: Arc, artifacts: Arc, + files: &dyn crate::files::FileSystem, reporter: &Reporter, ) -> Result<(), BuildManyError> { let mut futs = FuturesOrdered::new(); @@ -109,6 +114,7 @@ pub async fn build_many( &task, builder, artifacts, + files, ) .await; diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 0edc12d82..902346eda 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -10,7 +10,7 @@ use sha2::{Digest, Sha256}; use crate::{ Canister, CanisterArgs, canister::{ControllerRef, ManifestEnvVar, Settings, build::Build, wasm}, - fs, + files::FileSystem, manifest::{ ArgsFormat, BuildStep, BuildSteps, CanisterManifest, CanisterSelection, DependencyManifest, EnvironmentManifest, Instructions, Item, LoadManifestFromPathError, ManagedMode, @@ -119,7 +119,10 @@ pub enum BundleError { }, #[snafu(display("failed to read args file '{path}'"))] - ReadArgsFile { path: PathBuf, source: fs::IoError }, + ReadArgsFile { + path: PathBuf, + source: crate::files::FsError, + }, #[snafu(display( "failed to read the file backing environment variable '{variable}' of canister '{canister}'" @@ -127,7 +130,7 @@ pub enum BundleError { ReadEnvVar { canister: String, variable: String, - source: fs::IoError, + source: crate::files::FsError, }, #[snafu(display("failed to serialize bundle manifest"))] @@ -148,11 +151,8 @@ pub enum BundleError { #[snafu(display("failed to finalize bundle archive"))] FlushArchive { source: std::io::Error }, - #[snafu(display("failed to canonicalize path '{path}'"))] - CanonicalizePath { - path: PathBuf, - source: std::io::Error, - }, + #[snafu(display("failed to resolve the real location of '{path}'"))] + CanonicalizePath { path: PathBuf }, #[snafu(display( "source path '{path}' for canister '{canister}' resolves outside the project directory \ @@ -197,18 +197,21 @@ pub enum BundleError { #[snafu(display("failed to read plugin wasm for canister '{canister}'"))] ReadPlugin { canister: String, - source: fs::IoError, + source: crate::files::FsError, }, #[snafu(display("failed to read plugin file '{file}' for canister '{canister}'"))] ReadPluginFile { canister: String, file: String, - source: fs::IoError, + source: crate::files::FsError, }, #[snafu(display("failed to read app manifest '{path}'"))] - ReadAppManifest { path: PathBuf, source: fs::IoError }, + ReadAppManifest { + path: PathBuf, + source: crate::files::FsError, + }, #[snafu(display("failed to parse app manifest '{path}'"))] ParseAppManifest { @@ -241,7 +244,10 @@ pub enum BundleError { SerializeAppManifest { source: serde_yaml::Error }, #[snafu(display("failed to read image '{path}'"))] - ReadImage { path: PathBuf, source: fs::IoError }, + ReadImage { + path: PathBuf, + source: crate::files::FsError, + }, } /// In-memory bytes destined for a single tar entry. @@ -368,6 +374,7 @@ impl Pruned<'_> { } pub async fn create_bundle( + files: &dyn FileSystem, project_dir: &Path, canisters: Vec<(PathBuf, Canister)>, selected: &HashSet, @@ -395,31 +402,35 @@ pub async fn create_bundle( // dependency declarations — and the store keys and `PUBLIC_CANISTER_ID` // wiring deploy derives from them — carry over unchanged. let instances = group_canisters( - workspace_instances(project_dir).await?, + workspace_instances(files, project_dir).await?, &canisters, project_dir, )?; validate_canisters(&instances)?; let mut prefixes_by_dir: HashMap = HashMap::with_capacity(instances.len()); for instance in &instances { - prefixes_by_dir.insert(canonicalize(&instance.dir)?, instance.prefix.clone()); + prefixes_by_dir.insert( + canonicalize(files, &instance.dir).await?, + instance.prefix.clone(), + ); } let pruned = Pruned { dropped: &dropped, prefixes_by_dir: &prefixes_by_dir, environment, }; - let canonical_project_dir = canonicalize(project_dir)?; + let canonical_project_dir = canonicalize(files, project_dir).await?; let canonical_sync_dirs = validate_source_paths(project_dir, &canisters, &canonical_project_dir)?; - validate_env_var_files(&canisters, &canonical_project_dir)?; - validate_output_path(output, &canonical_sync_dirs)?; + validate_env_var_files(files, &canisters, &canonical_project_dir).await?; + validate_output_path(files, output, &canonical_sync_dirs).await?; build_many( canisters.clone(), environment, builder, artifacts.clone(), + files, reporter, ) .await?; @@ -452,6 +463,7 @@ pub async fn create_bundle( for instance in &instances { let canister_items = prepare_canisters( + files, instance, &pruned, &*artifacts, @@ -459,8 +471,9 @@ pub async fn create_bundle( &mut bundle_artifacts, ) .await?; - let networks = inline_networks(&instance.manifest.networks, &instance.dir).await?; + let networks = inline_networks(files, &instance.manifest.networks, &instance.dir).await?; let environments = inline_environments( + files, instance, &pruned, &canonical_project_dir, @@ -484,15 +497,17 @@ pub async fn create_bundle( }); } - let app_manifest = prepare_app_manifest(project_dir, &canonical_project_dir)?; + let app_manifest = prepare_app_manifest(files, project_dir, &canonical_project_dir).await?; write_archive( + files, output, &manifests, &bundle_artifacts, &args_files, app_manifest.as_ref(), ) + .await } /// The local name a canister's owning project knows it by: consolidation keys a @@ -671,6 +686,7 @@ fn group_canisters( /// Build one instance's manifest items and collect the archive artifacts they reference. async fn prepare_canisters( + files: &dyn FileSystem, instance: &Instance, pruned: &Pruned<'_>, artifacts: &dyn store_artifact::Access, @@ -688,6 +704,7 @@ async fn prepare_canisters( let mut items = Vec::with_capacity(instance.canisters.len()); for (canister_path, canister) in &instance.canisters { let item = prepare_canister( + files, &instance.prefix, canister_path, canister, @@ -705,6 +722,7 @@ async fn prepare_canisters( #[allow(clippy::too_many_arguments)] async fn prepare_canister( + files: &dyn FileSystem, prefix: &str, canister_path: &Path, canister: &Canister, @@ -745,6 +763,7 @@ async fn prepare_canister( plugin_idx += 1; bundle_sync_steps.push( prepare_plugin_step( + files, adapter, prefix, canister, @@ -854,6 +873,7 @@ fn localize_call_targets( #[allow(clippy::too_many_arguments)] async fn prepare_plugin_step( + files: &dyn FileSystem, adapter: &plugin::Adapter, prefix: &str, canister: &Canister, @@ -878,7 +898,7 @@ async fn prepare_plugin_step( canister: canister.name.clone(), })?; - let plugin_bytes = fs::read(&resolved).context(ReadPluginSnafu { + let plugin_bytes = files.read(&resolved).await.context(ReadPluginSnafu { canister: canister.name.clone(), })?; let plugin_sha256 = hex::encode(Sha256::digest(&plugin_bytes)); @@ -918,9 +938,15 @@ async fn prepare_plugin_step( // A `files:` entry names a directory or a file, and which it is comes from what is on // disk — the same rule the plugin host applies. So partition on that before deciding // whether the archive gets a tree or a single file. - let (file_dirs, file_files): (Vec, Vec) = declared(&adapter.files) - .into_iter() - .partition(|path| canister_path.join(path).is_dir()); + let mut file_dirs: Vec = Vec::new(); + let mut file_files: Vec = Vec::new(); + for path in declared(&adapter.files) { + if files.is_dir(&canister_path.join(&path)).await { + file_dirs.push(path); + } else { + file_files.push(path); + } + } for (dir, dir_prefix) in covering_dirs(declared(&adapter.dirs).iter().map(String::as_str)) .into_iter() @@ -964,6 +990,7 @@ async fn prepare_plugin_step( } async fn inline_networks( + files: &dyn FileSystem, items: &[Item], instance_dir: &Path, ) -> Result>, BundleError> { @@ -973,7 +1000,7 @@ async fn inline_networks( Item::Manifest(_) => item.clone(), Item::Path(path) => { let full = instance_dir.join(path); - let m = load_manifest_from_path::(&full) + let m = load_manifest_from_path::(files, &full) .await .context(LoadNetworkSnafu { path: full })?; Item::Manifest(m) @@ -1023,7 +1050,8 @@ const UPGRADE_ARGS_DIR: &str = "upgrade-args"; /// Relocate the files one environment's args overrides point at into /// `archive_dir`, rewriting each override to name the archived copy. #[allow(clippy::too_many_arguments)] -fn relocate_args_overrides( +async fn relocate_args_overrides( + files: &dyn FileSystem, overrides: &mut HashMap, archive_dir: &str, instance_prefix: &str, @@ -1049,7 +1077,7 @@ fn relocate_args_overrides( // could otherwise point the args at host files outside the project, and // normalize_archive_dir would silently strip any leading `..` from the // rewritten archive path so the escape wouldn't be visible there. - canonicalize_within_project(&src, canonical_project_dir, canister_name)?; + canonicalize_within_project(files, &src, canonical_project_dir, canister_name).await?; let manifest_path = format!( "{archive_dir}/{}/{}", path_segment(canister_name), @@ -1080,6 +1108,7 @@ fn relocate_args_overrides( #[allow(clippy::too_many_arguments)] async fn inline_environments( + files: &dyn FileSystem, instance: &Instance, pruned: &Pruned<'_>, canonical_project_dir: &Path, @@ -1098,7 +1127,7 @@ async fn inline_environments( Item::Manifest(_) => item.clone(), Item::Path(path) => { let full = instance_dir.join(path); - let m = load_manifest_from_path::(&full) + let m = load_manifest_from_path::(files, &full) .await .context(LoadEnvironmentSnafu { path: full })?; Item::Manifest(m) @@ -1119,6 +1148,7 @@ async fn inline_environments( ] { let Some(overrides) = overrides else { continue }; relocate_args_overrides( + files, overrides, archive_dir, instance_prefix, @@ -1128,7 +1158,8 @@ async fn inline_environments( owner_prefixes, seen_archive_paths, args_files, - )?; + ) + .await?; } } @@ -1151,7 +1182,7 @@ async fn inline_environments( // Same containment rule as an init_args override above: a // manifest must not have the bundle carry off a file from // outside the project, inlined value or archived copy. - let canon = canonicalize(&src)?; + let canon = canonicalize(files, &src).await?; if !canon.starts_with(canonical_project_dir) { return EnvVarEscapesProjectSnafu { canister: canister_name.clone(), @@ -1161,7 +1192,7 @@ async fn inline_environments( } .fail(); } - let value = fs::read_to_string(&src).context(ReadEnvVarSnafu { + let value = files.read_to_string(&src).await.context(ReadEnvVarSnafu { canister: canister_name, variable, })?; @@ -1221,18 +1252,22 @@ fn prune_environment(env: &mut EnvironmentManifest, instance: &Instance, pruned: /// Load `icp_appmanifest.yaml` if present, rewriting its top-level `images` paths to point at /// copies relocated under `images/` in the bundle. Returns `None` when the file is absent. -fn prepare_app_manifest( +async fn prepare_app_manifest( + files: &dyn FileSystem, project_dir: &Path, canonical_project_dir: &Path, ) -> Result, BundleError> { let manifest_path = project_dir.join(APP_MANIFEST); - if !manifest_path.exists() { + if !files.exists(&manifest_path).await { return Ok(None); } - let raw = fs::read_to_string(&manifest_path).context(ReadAppManifestSnafu { - path: &manifest_path, - })?; + let raw = files + .read_to_string(&manifest_path) + .await + .context(ReadAppManifestSnafu { + path: &manifest_path, + })?; let mut doc: serde_yaml::Value = serde_yaml::from_str(&raw).context(ParseAppManifestSnafu { path: &manifest_path, })?; @@ -1263,7 +1298,7 @@ fn prepare_app_manifest( })? .to_owned(); let src = project_dir.join(&orig); - let canon = canonicalize(&src)?; + let canon = canonicalize(files, &src).await?; if !canon.starts_with(canonical_project_dir) { return ImageEscapesProjectSnafu { path: src, @@ -1391,7 +1426,8 @@ impl ArchiveWriter { } } -fn write_archive( +async fn write_archive( + files: &dyn FileSystem, output: &Path, manifests: &[InstanceManifest], artifacts: &BundleArtifacts, @@ -1440,7 +1476,7 @@ fn write_archive( if let Some(app) = app_manifest { archive.bytes(APP_MANIFEST, app.yaml.as_bytes())?; for shot in &app.images { - let data = fs::read(&shot.src_path).context(ReadImageSnafu { + let data = files.read(&shot.src_path).await.context(ReadImageSnafu { path: shot.src_path.clone(), })?; archive.bytes(&shot.archive_path, &data)?; @@ -1452,9 +1488,12 @@ fn write_archive( } for entry in args_files { - let data = fs::read(&entry.src_path).context(ReadArgsFileSnafu { - path: entry.src_path.clone(), - })?; + let data = files + .read(&entry.src_path) + .await + .context(ReadArgsFileSnafu { + path: entry.src_path.clone(), + })?; archive.bytes(&entry.archive_path, &data)?; } @@ -1467,10 +1506,13 @@ fn write_archive( } for pf in &artifacts.plugin_files { - let data = fs::read(&pf.src_path).context(ReadPluginFileSnafu { - canister: pf.canister_name.clone(), - file: pf.orig_file.clone(), - })?; + let data = files + .read(&pf.src_path) + .await + .context(ReadPluginFileSnafu { + canister: pf.canister_name.clone(), + file: pf.orig_file.clone(), + })?; archive.bytes(&pf.archive_path, &data)?; } @@ -1579,13 +1621,14 @@ fn validate_source_paths( /// settings before `create_bundle` runs — so the paths come from the canister /// model rather than the manifest, and the environment overrides `create_bundle` /// rewrites itself are checked in [`inline_environments`]. -fn validate_env_var_files( +async fn validate_env_var_files( + files: &dyn FileSystem, canisters: &[(PathBuf, Canister)], canonical_project_dir: &Path, ) -> Result<(), BundleError> { for (_, canister) in canisters { for (variable, file) in &canister.environment_variable_files { - let canon = canonicalize(file)?; + let canon = canonicalize(files, file).await?; if !canon.starts_with(canonical_project_dir) { return EnvVarEscapesProjectSnafu { canister: canister.name.clone(), @@ -1655,8 +1698,12 @@ fn resolve_within_project( /// Refuse to write the bundle output into a directory we are about to recursively archive — /// otherwise the partial bundle file would be included in itself. -fn validate_output_path(output: &Path, canonical_sync_dirs: &[PathBuf]) -> Result<(), BundleError> { - let canonical_output = canonicalize_output(output)?; +async fn validate_output_path( + files: &dyn FileSystem, + output: &Path, + canonical_sync_dirs: &[PathBuf], +) -> Result<(), BundleError> { + let canonical_output = canonicalize_output(files, output).await?; for sync_dir in canonical_sync_dirs { if canonical_output.starts_with(sync_dir) { return OutputOverlapsSyncDirSnafu { @@ -1710,18 +1757,22 @@ fn is_absolute_bind_mount_host(mount: &str) -> bool { !h.is_empty() && (h[0] == b'/' || h[0] == b'\\') } -fn canonicalize(path: &Path) -> Result { - path.canonicalize_utf8().context(CanonicalizePathSnafu { - path: path.to_path_buf(), - }) +async fn canonicalize(files: &dyn FileSystem, path: &Path) -> Result { + files + .canonicalize(path) + .await + .context(CanonicalizePathSnafu { + path: path.to_path_buf(), + }) } -fn canonicalize_within_project( +async fn canonicalize_within_project( + files: &dyn FileSystem, src: &Path, canonical_project_dir: &Path, canister: &str, ) -> Result { - let canon = canonicalize(src)?; + let canon = canonicalize(files, src).await?; if !canon.starts_with(canonical_project_dir) { return SourceEscapesProjectSnafu { canister: canister.to_owned(), @@ -1735,9 +1786,12 @@ fn canonicalize_within_project( /// Resolve the canonical form of an output path that may not exist yet. We canonicalize its /// parent (which must exist before we can write a file there anyway) and append the filename. -fn canonicalize_output(output: &Path) -> Result { - if output.exists() { - return canonicalize(output); +async fn canonicalize_output( + files: &dyn FileSystem, + output: &Path, +) -> Result { + if files.exists(output).await { + return canonicalize(files, output).await; } let parent = output .parent() @@ -1747,7 +1801,7 @@ fn canonicalize_output(output: &Path) -> Result { .file_name() .map(|s| s.to_string()) .unwrap_or_default(); - let canon_parent = canonicalize(parent)?; + let canon_parent = canonicalize(files, parent).await?; Ok(canon_parent.join(filename)) } diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index cc560a10b..76346866a 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -238,6 +238,7 @@ pub async fn deploy( environment_selection.name(), host.builder.clone(), host.artifacts.clone(), + host.files.as_ref(), &phase.reporter(), ) .await; @@ -652,7 +653,14 @@ pub async fn resolve_targets( // project load.) let project = host.project.load().await?; let member_dir = host.project.member_dir(); - match crate::project::member_scoped_canisters(&project.dir, member_dir.as_deref(), &env) { + match crate::project::member_scoped_canisters( + host.files.as_ref(), + &project.dir, + member_dir.as_deref(), + &env, + ) + .await + { Some(scoped) => { member_scoped = true; scoped diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index 8eb20156b..954a6fb82 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -7,7 +7,7 @@ use snafu::prelude::*; use crate::{ Canister, CanisterArgs, Environment, Network, Project, canister::{ControllerRef, ManifestEnvVar, ManifestSettings, Settings, recipe}, - fs, + files::{FileSystem, expand_glob}, manifest::{ ArgsFormat, CANISTER_MANIFEST, CanisterManifest, DependencyManifest, EnvironmentManifest, Item, LoadManifestFromPathError, ManifestArgs, NetworkManifest, PROJECT_MANIFEST, @@ -50,11 +50,8 @@ pub enum ConsolidateManifestError { #[snafu(display("failed to locate project directory"))] Locate { source: ProjectRootLocateError }, - #[snafu(display("failed to perform glob parsing"))] - GlobParse { source: glob::PatternError }, - - #[snafu(display("failed to get glob iter"))] - GlobIter { source: glob::GlobError }, + #[snafu(display("failed to expand a canister path glob"))] + ExpandGlob { source: crate::files::GlobError }, #[snafu(display("failed to convert path to UTF-8"))] Utf8Path { source: FromPathBufError }, @@ -103,7 +100,7 @@ pub enum ConsolidateManifestError { #[snafu(display("failed to read {field} file for canister '{canister}'"))] ReadArgs { - source: fs::IoError, + source: crate::files::FsError, canister: String, field: ArgsField, }, @@ -112,7 +109,7 @@ pub enum ConsolidateManifestError { "failed to read the file backing environment variable '{variable}' of canister '{canister}'" ))] ReadEnvironmentVariable { - source: fs::IoError, + source: crate::files::FsError, canister: String, variable: String, }, @@ -208,7 +205,8 @@ impl std::fmt::Display for ArgsField { /// Resolve a [`ManifestArgs`] into a canonical [`CanisterArgs`] by reading /// any file references relative to `base_path`. -fn resolve_manifest_args( +async fn resolve_manifest_args( + files: &dyn FileSystem, manifest_args: &ManifestArgs, base_path: &Path, canister: &str, @@ -223,11 +221,16 @@ fn resolve_manifest_args( let file_path = base_path.join(path); match format { ArgsFormat::Bin => { - let bytes = fs::read(&file_path).context(ReadArgsSnafu { canister, field })?; + let bytes = files + .read(&file_path) + .await + .context(ReadArgsSnafu { canister, field })?; Ok(CanisterArgs::Binary(bytes)) } fmt => { - let content = fs::read_to_string(&file_path) + let content = files + .read_to_string(&file_path) + .await .context(ReadArgsSnafu { canister, field })?; Ok(CanisterArgs::Text { content: content.trim().to_owned(), @@ -250,7 +253,8 @@ fn resolve_manifest_args( /// reading any file-backed environment variable values relative to `base_path`. /// Also returns the file each such value came from, for /// [`Canister::environment_variable_files`]. -fn resolve_manifest_settings( +async fn resolve_manifest_settings( + files: &dyn FileSystem, manifest_settings: &ManifestSettings, base_path: &Path, canister: &str, @@ -270,31 +274,31 @@ fn resolve_manifest_settings( controllers, } = manifest_settings; - let mut files = BTreeMap::new(); - let environment_variables = environment_variables - .as_ref() - .map(|vars| { - vars.iter() - .map(|(name, var)| { - let value = match var { - ManifestEnvVar::Value(value) => value.to_owned(), - ManifestEnvVar::Path { path } => { - let file = base_path.join(path); - let contents = fs::read_to_string(&file).context( - ReadEnvironmentVariableSnafu { - canister, - variable: name, - }, - )?; - files.insert(name.to_owned(), file); - contents.trim().to_owned() - } - }; - Ok((name.to_owned(), value)) - }) - .collect::, ConsolidateManifestError>>() - }) - .transpose()?; + let mut var_files = BTreeMap::new(); + let environment_variables = match environment_variables.as_ref() { + None => None, + Some(vars) => { + let mut resolved = HashMap::with_capacity(vars.len()); + for (name, var) in vars { + let value = match var { + ManifestEnvVar::Value(value) => value.to_owned(), + ManifestEnvVar::Path { path } => { + let file = base_path.join(path); + let contents = files.read_to_string(&file).await.context( + ReadEnvironmentVariableSnafu { + canister, + variable: name, + }, + )?; + var_files.insert(name.to_owned(), file); + contents.trim().to_owned() + } + }; + resolved.insert(name.to_owned(), value); + } + Some(resolved) + } + }; let settings = Settings { log_visibility: log_visibility.clone(), @@ -310,7 +314,7 @@ fn resolve_manifest_settings( environment_variables, controllers: controllers.clone(), }; - Ok((settings, files)) + Ok((settings, var_files)) } fn is_glob(s: &str) -> bool { @@ -338,6 +342,7 @@ fn is_valid_name(name: &str) -> bool { /// callers assign store keys and bindings. Does not check for duplicate names /// across projects — that is the caller's responsibility (via the global map). async fn build_manifest_canisters( + files: &dyn FileSystem, pdir: &Path, manifest_canisters: &[Item], recipe_resolver: &dyn recipe::Resolve, @@ -353,27 +358,21 @@ async fn build_manifest_canisters( false => vec![pdir.join(pattern)], // Glob pattern - true => { - let paths = - glob::glob(pdir.join(pattern).as_str()).context(GlobParseSnafu)?; - - let mut v = vec![]; - for p in paths { - let path = p.context(GlobIterSnafu)?; - let utf8_path = PathBuf::try_from(path).context(Utf8PathSnafu)?; - v.push(utf8_path); - } - v - } + true => expand_glob(files, pdir, pattern) + .await + .context(ExpandGlobSnafu)?, }; let paths = if is_glob_pattern { // For glob patterns, filter out non-directories and non-canister directories - paths - .into_iter() - .filter(|p| p.is_dir()) - .filter(|p| p.join(CANISTER_MANIFEST).exists()) - .collect::>() + let mut kept = Vec::new(); + for p in paths { + if files.is_dir(&p).await && files.exists(&p.join(CANISTER_MANIFEST)).await + { + kept.push(p); + } + } + kept } else { // For explicit paths, validate that they exist and contain canister.yaml let mut validated_paths = vec![]; @@ -394,9 +393,12 @@ async fn build_manifest_canisters( for p in paths { ms.push(( p.to_owned(), - load_manifest_from_path::(&p.join(CANISTER_MANIFEST)) - .await - .context(LoadCanisterSnafu)?, + load_manifest_from_path::( + files, + &p.join(CANISTER_MANIFEST), + ) + .await + .context(LoadCanisterSnafu)?, )); } ms @@ -473,19 +475,21 @@ async fn build_manifest_canisters( }; let (settings, environment_variable_files) = - resolve_manifest_settings(&m.settings, &cdir, &m.name)?; + resolve_manifest_settings(files, &m.settings, &cdir, &m.name).await?; - let init_args = m - .init_args - .as_ref() - .map(|ma| resolve_manifest_args(ma, &cdir, &m.name, ArgsField::Init)) - .transpose()?; + let init_args = match m.init_args.as_ref() { + Some(ma) => { + Some(resolve_manifest_args(files, ma, &cdir, &m.name, ArgsField::Init).await?) + } + None => None, + }; - let upgrade_args = m - .upgrade_args - .as_ref() - .map(|ma| resolve_manifest_args(ma, &cdir, &m.name, ArgsField::Upgrade)) - .transpose()?; + let upgrade_args = match m.upgrade_args.as_ref() { + Some(ma) => Some( + resolve_manifest_args(files, ma, &cdir, &m.name, ArgsField::Upgrade).await?, + ), + None => None, + }; result.push(( m.name.clone(), @@ -604,7 +608,11 @@ fn own_canisters_left_out( /// Canonicalize a dependency root (resolving symlinks and `..`) for use as a /// de-dup / cycle-detection identity. -fn canonicalize_dep(alias: &str, dep_root: &Path) -> Result { +async fn canonicalize_dep( + files: &dyn FileSystem, + alias: &str, + dep_root: &Path, +) -> Result { let build_err = || { DependencyCanonicalizeSnafu { alias: alias.to_owned(), @@ -612,8 +620,7 @@ fn canonicalize_dep(alias: &str, dep_root: &Path) -> Result Result, WorkspaceInstancesError> { let root_manifest_path = pdir.join(PROJECT_MANIFEST); - let root_manifest: ProjectManifest = load_manifest_from_path(&root_manifest_path) + let root_manifest: ProjectManifest = load_manifest_from_path(files, &root_manifest_path) .await .context(LoadWorkspaceRootSnafu { path: &root_manifest_path, @@ -747,9 +759,11 @@ pub async fn workspace_instances( // Same fallback as `consolidate_manifest`, so prefixes agree with the store // keys even when the root directory cannot be canonicalized. - let app_root_canonical = canonicalize_or(pdir).unwrap_or_else(|| pdir.to_owned()); + let app_root_canonical = canonicalize_or(files, pdir) + .await + .unwrap_or_else(|| pdir.to_owned()); - let root_edges = resolve_edges(pdir, &root_manifest, &app_root_canonical)?; + let root_edges = resolve_edges(files, pdir, &root_manifest, &app_root_canonical).await?; // Depth-first, declaration order: push each instance's dependencies reversed // so the top of the stack is always the next edge in manifest order. @@ -769,15 +783,14 @@ pub async fn workspace_instances( } let manifest_path = edge.dir.join(PROJECT_MANIFEST); - let manifest: ProjectManifest = - load_manifest_from_path(&manifest_path) - .await - .context(LoadInstanceSnafu { - alias: &edge.alias, - path: &manifest_path, - })?; + let manifest: ProjectManifest = load_manifest_from_path(files, &manifest_path) + .await + .context(LoadInstanceSnafu { + alias: &edge.alias, + path: &manifest_path, + })?; - let edges = resolve_edges(&edge.dir, &manifest, &app_root_canonical)?; + let edges = resolve_edges(files, &edge.dir, &manifest, &app_root_canonical).await?; let dependency_prefixes = edges.iter().map(|e| e.prefix.clone()).collect(); stack.extend(edges.into_iter().rev()); @@ -913,6 +926,7 @@ fn validate_dependency_aliases( /// instance's prefix and its own canisters. #[allow(clippy::too_many_arguments)] async fn import_dependency( + files: &dyn FileSystem, app_root_canonical: &Path, parent_dir: &Path, dep: &DependencyManifest, @@ -936,7 +950,7 @@ async fn import_dependency( .fail(); } - let canonical = canonicalize_dep(&dep.name, &dep_root)?; + let canonical = canonicalize_dep(files, &dep.name, &dep_root).await?; // Cycle detection. if stack.contains(&canonical) { @@ -973,18 +987,18 @@ async fn import_dependency( let prefix = relative_prefix(app_root_canonical, &canonical); - let dep_manifest: ProjectManifest = - load_manifest_from_path(&manifest_path) - .await - .context(LoadDependencyManifestSnafu { - alias: dep.name.clone(), - })?; + let dep_manifest: ProjectManifest = load_manifest_from_path(files, &manifest_path) + .await + .context(LoadDependencyManifestSnafu { + alias: dep.name.clone(), + })?; // Build the dependency's own canisters and key them under the prefix. All of // them are imported (deploy-all); the `canisters` exposure subset is applied // by the caller when wiring env vars. let built = - build_manifest_canisters(&dep_root, &dep_manifest.canisters, recipe_resolver).await?; + build_manifest_canisters(files, &dep_root, &dep_manifest.canisters, recipe_resolver) + .await?; let mut own: Vec<(String, String)> = Vec::new(); let mut local_to_key: BTreeMap = BTreeMap::new(); @@ -1035,7 +1049,7 @@ async fn import_dependency( } .fail(); } - load_manifest_from_path::(&p) + load_manifest_from_path::(files, &p) .await .context(LoadEnvironmentSnafu)? } @@ -1133,6 +1147,7 @@ async fn import_dependency( let mut nested_chain = alias_chain.to_vec(); nested_chain.push(nested.name.clone()); let inst = Box::pin(import_dependency( + files, app_root_canonical, &dep_root, nested, @@ -1170,9 +1185,8 @@ async fn import_dependency( } /// Canonicalize into a UTF-8 path, or `None` if it does not exist / is not UTF-8. -fn canonicalize_or(dir: &Path) -> Option { - let canon = dunce::canonicalize(dir.as_std_path()).ok()?; - PathBuf::try_from(canon).ok() +async fn canonicalize_or(files: &dyn FileSystem, dir: &Path) -> Option { + files.canonicalize(dir).await } /// The default set of target canisters when the user names none, honoring @@ -1188,26 +1202,28 @@ fn canonicalize_or(dir: &Path) -> Option { /// Returns `None` meaning "no scoping — target the whole environment": at the /// workspace root or a standalone project (`member_dir` resolves to `root_dir`), /// when `member_dir` is unknown, or when paths cannot be resolved. -pub fn member_scoped_canisters( +pub async fn member_scoped_canisters( + files: &dyn FileSystem, root_dir: &Path, member_dir: Option<&Path>, env: &Environment, ) -> Option> { let member = member_dir?; - let root_c = canonicalize_or(root_dir)?; - let member_c = canonicalize_or(member)?; + let root_c = canonicalize_or(files, root_dir).await?; + let member_c = canonicalize_or(files, member).await?; if root_c == member_c { return None; } - let names = env - .canisters - .iter() - .filter(|(_, (dir, _))| { - canonicalize_or(dir).is_some_and(|c| c == member_c || c.starts_with(&member_c)) - }) - .map(|(name, _)| name.clone()) - .collect(); + let mut names = Vec::new(); + for (name, (dir, _)) in &env.canisters { + if canonicalize_or(files, dir) + .await + .is_some_and(|c| c == member_c || c.starts_with(&member_c)) + { + names.push(name.clone()); + } + } Some(names) } @@ -1219,7 +1235,8 @@ pub fn member_scoped_canisters( /// Membership, unlike those overrides, has no precedence to resolve: each project /// decides its own canisters and only its own, so `root_left_out` and every /// member's contribution address disjoint sets of keys. -fn build_environment_canisters( +async fn build_environment_canisters( + files: &dyn FileSystem, canisters: &IndexMap, root_left_out: &HashSet, member: Option<&MemberEnvContribution>, @@ -1243,15 +1260,16 @@ fn build_environment_canisters( if let Some((cpath, canister)) = cs.get_mut(key) { if let Some(s) = &ov.settings { (canister.settings, canister.environment_variable_files) = - resolve_manifest_settings(s, cpath, key)?; + resolve_manifest_settings(files, s, cpath, key).await?; } if let Some(ia) = &ov.init_args { canister.init_args = - Some(resolve_manifest_args(ia, cpath, key, ArgsField::Init)?); + Some(resolve_manifest_args(files, ia, cpath, key, ArgsField::Init).await?); } if let Some(ua) = &ov.upgrade_args { - canister.upgrade_args = - Some(resolve_manifest_args(ua, cpath, key, ArgsField::Upgrade)?); + canister.upgrade_args = Some( + resolve_manifest_args(files, ua, cpath, key, ArgsField::Upgrade).await?, + ); } } } @@ -1262,14 +1280,15 @@ fn build_environment_canisters( for (name, s) in settings { if let Some((cpath, canister)) = cs.get_mut(name) { (canister.settings, canister.environment_variable_files) = - resolve_manifest_settings(s, cpath, name)?; + resolve_manifest_settings(files, s, cpath, name).await?; } } } if let Some(init_args) = root_init_args { for (name, ia) in init_args { if let Some((cpath, canister)) = cs.get_mut(name) { - canister.init_args = Some(resolve_manifest_args(ia, cpath, name, ArgsField::Init)?); + canister.init_args = + Some(resolve_manifest_args(files, ia, cpath, name, ArgsField::Init).await?); } } } @@ -1277,7 +1296,7 @@ fn build_environment_canisters( for (name, ua) in upgrade_args { if let Some((cpath, canister)) = cs.get_mut(name) { canister.upgrade_args = - Some(resolve_manifest_args(ua, cpath, name, ArgsField::Upgrade)?); + Some(resolve_manifest_args(files, ua, cpath, name, ArgsField::Upgrade).await?); } } } @@ -1295,6 +1314,7 @@ fn build_environment_canisters( /// - All the referenced canisters exist /// - All the recipes have been resolved pub async fn consolidate_manifest( + files: &dyn FileSystem, pdir: &Path, recipe_resolver: &dyn recipe::Resolve, m: &ProjectManifest, @@ -1305,11 +1325,12 @@ pub async fn consolidate_manifest( // Canonical app root, used to derive stable, order-independent store-key // prefixes for imported dependency canisters. - let app_root_canonical = - canonicalize_dep("", pdir).unwrap_or_else(|_| pdir.to_owned()); + let app_root_canonical = canonicalize_dep(files, "", pdir) + .await + .unwrap_or_else(|_| pdir.to_owned()); // This project's own canisters, keyed by their bare local names. - let app_built = build_manifest_canisters(pdir, &m.canisters, recipe_resolver).await?; + let app_built = build_manifest_canisters(files, pdir, &m.canisters, recipe_resolver).await?; let mut app_own: Vec<(String, String)> = Vec::new(); for (local, cdir, canister) in app_built { app_own.push((local.clone(), local.clone())); @@ -1342,6 +1363,7 @@ pub async fn consolidate_manifest( let mut app_edges: Vec<(String, Vec<(String, String)>)> = Vec::new(); for dep in &m.dependencies { let inst = import_dependency( + files, &app_root_canonical, pdir, dep, @@ -1423,7 +1445,7 @@ pub async fn consolidate_manifest( } .fail(); } - load_manifest_from_path::(&path) + load_manifest_from_path::(files, &path) .await .context(LoadNetworkSnafu)? } @@ -1503,7 +1525,7 @@ pub async fn consolidate_manifest( } .fail(); } - load_manifest_from_path::(&path) + load_manifest_from_path::(files, &path) .await .context(LoadEnvironmentSnafu)? } @@ -1546,13 +1568,15 @@ pub async fn consolidate_manifest( // Embed canisters in environment, folding each member's own // membership and overrides in beneath the root's. let built = build_environment_canisters( + files, &canisters, &left_out, member_envs.get(&m.name), m.settings.as_ref(), m.init_args.as_ref(), m.upgrade_args.as_ref(), - )?; + ) + .await?; e.insert(Environment { name: m.name.to_owned(), network, @@ -1576,13 +1600,15 @@ pub async fn consolidate_manifest( )? .to_owned(); let built = build_environment_canisters( + files, &canisters, &HashSet::new(), member_envs.get(LOCAL), None, None, None, - )?; + ) + .await?; vacant_entry.insert(Environment { name: LOCAL.to_string(), network, @@ -1601,13 +1627,15 @@ pub async fn consolidate_manifest( )? .to_owned(); let built = build_environment_canisters( + files, &canisters, &HashSet::new(), member_envs.get(IC), None, None, None, - )?; + ) + .await?; vacant_entry.insert(Environment { name: IC.to_string(), network, @@ -1648,6 +1676,7 @@ pub async fn consolidate_manifest( mod recipe_sync_tests { use super::*; use crate::canister::recipe::{Fetched, Resolve, ResolveError}; + use crate::files::HostFileSystem; use crate::manifest::canister::SyncStep; use crate::manifest::recipe::Recipe; use camino_tempfile::Utf8TempDir; @@ -1678,10 +1707,11 @@ mod recipe_sync_tests { "#}; async fn consolidate(pdir: &Path) -> Result { - let m: ProjectManifest = load_manifest_from_path(&pdir.join(PROJECT_MANIFEST)) - .await - .expect("failed to parse project manifest"); - consolidate_manifest(pdir, &FixedResolver(TEMPLATE), &m).await + let m: ProjectManifest = + load_manifest_from_path(&HostFileSystem, &pdir.join(PROJECT_MANIFEST)) + .await + .expect("failed to parse project manifest"); + consolidate_manifest(&HostFileSystem, pdir, &FixedResolver(TEMPLATE), &m).await } /// The commands of a canister's sync steps, which are all script steps here. @@ -1754,6 +1784,7 @@ mod recipe_sync_tests { mod dependency_tests { use super::*; use crate::canister::recipe::{Fetched, Resolve, ResolveError}; + use crate::files::HostFileSystem; use crate::manifest::recipe::Recipe; use camino_tempfile::Utf8TempDir; @@ -1792,10 +1823,11 @@ mod dependency_tests { } async fn consolidate(pdir: &Path) -> Result { - let m: ProjectManifest = load_manifest_from_path(&pdir.join(PROJECT_MANIFEST)) - .await - .expect("failed to parse project manifest"); - consolidate_manifest(pdir, &PanicResolver, &m).await + let m: ProjectManifest = + load_manifest_from_path(&HostFileSystem, &pdir.join(PROJECT_MANIFEST)) + .await + .expect("failed to parse project manifest"); + consolidate_manifest(&HostFileSystem, pdir, &PanicResolver, &m).await } fn bindings_of<'a>(p: &'a Project, key: &str) -> &'a BTreeMap { @@ -1913,14 +1945,21 @@ mod dependency_tests { let env = p.environments.get(LOCAL).expect("local environment"); // At the workspace root (member == root): no scoping. - assert_eq!(member_scoped_canisters(&p.dir, Some(&p.dir), env), None); + assert_eq!( + member_scoped_canisters(&HostFileSystem, &p.dir, Some(&p.dir), env).await, + None + ); // Unknown member dir: no scoping. - assert_eq!(member_scoped_canisters(&p.dir, None, env), None); + assert_eq!( + member_scoped_canisters(&HostFileSystem, &p.dir, None, env).await, + None + ); // Inside the member: only the member's own canisters, not the app's. let member = tmp.path().join("openemail"); - let mut scoped = member_scoped_canisters(&p.dir, Some(&member), env) + let mut scoped = member_scoped_canisters(&HostFileSystem, &p.dir, Some(&member), env) + .await .expect("should scope when inside a member"); scoped.sort(); assert_eq!( diff --git a/crates/icp-project/src/store_artifact.rs b/crates/icp-project/src/store_artifact.rs index e5bc6fc40..5bd835aac 100644 --- a/crates/icp-project/src/store_artifact.rs +++ b/crates/icp-project/src/store_artifact.rs @@ -2,17 +2,17 @@ use std::sync::Arc; #[cfg(any(test, feature = "test-util"))] use std::{collections::HashMap, sync::Mutex}; +#[cfg(feature = "host")] use crate::{ CACHE_DIR, ICP_BASE, fs::{ lock::{DirectoryStructureLock, PathsAccess}, read, write, }, - manifest::ProjectRootLocate, - prelude::*, }; +use crate::{manifest::ProjectRootLocate, prelude::*, store_id::StoreCause}; use async_trait::async_trait; -use snafu::{ResultExt, Snafu}; +use snafu::Snafu; #[async_trait] /// Trait for accessing and managing canister build artifacts. @@ -26,24 +26,23 @@ pub trait Access: Sync + Send { #[derive(Debug, Snafu)] pub enum SaveError { - #[snafu(display("failed to write artifact file"))] - SaveWriteFileError { source: crate::fs::IoError }, - #[snafu(display( "canister '{name}' encodes to a {len}-byte artifact filename, exceeding the 255-byte \ filesystem limit; shorten the dependency path or canister name" ))] SaveNameTooLong { name: String, len: usize }, - #[snafu(transparent)] - LockError { source: crate::fs::lock::LockError }, + /// The store could not keep the artifact. What a store is made of is the + /// implementation's business, so the cause is carried whole. + #[snafu(display("failed to store the build artifact for canister '{name}'"))] + SaveStore { + source: crate::store_id::StoreCause, + name: String, + }, } #[derive(Debug, Snafu)] pub enum LookupArtifactError { - #[snafu(display("failed to read artifact file"))] - LookupReadFileError { source: crate::fs::IoError }, - #[snafu(display("could not find artifact for canister '{name}'"))] LookupArtifactNotFound { name: String }, @@ -53,14 +52,20 @@ pub enum LookupArtifactError { ))] LookupNameTooLong { name: String, len: usize }, - #[snafu(transparent)] - LockError { source: crate::fs::lock::LockError }, + /// As [`SaveError::SaveStore`]. + #[snafu(display("failed to read the build artifact for canister '{name}'"))] + LookupStore { + source: crate::store_id::StoreCause, + name: String, + }, } +#[cfg(feature = "host")] pub struct ArtifactStore { project_root_locate: Arc, } +#[cfg(feature = "host")] pub struct ArtifactPaths { dir: PathBuf, } @@ -72,6 +77,7 @@ pub struct ArtifactPaths { /// filename characters on every platform. Percent-encoding the unsafe set keeps /// the mapping reversible and collision-free; plain names (alphanumeric/`-`/`_`/`.`) /// are left unchanged, so existing artifact filenames are unaffected. +#[cfg(feature = "host")] fn sanitize_artifact_name(name: &str) -> String { let mut out = String::with_capacity(name.len()); for c in name.chars() { @@ -87,28 +93,34 @@ fn sanitize_artifact_name(name: &str) -> String { } /// Maximum length of a single filename component on common filesystems. +#[cfg(feature = "host")] const NAME_MAX: usize = 255; /// The encoded filename length if it exceeds `NAME_MAX`, else `None`. A deeply /// nested dependency store key can stay within the total path limit yet blow the /// per-component limit once its separators are percent-encoded. +#[cfg(feature = "host")] +#[cfg(feature = "host")] fn artifact_name_overflow(name: &str) -> Option { let len = sanitize_artifact_name(name).len(); (len > NAME_MAX).then_some(len) } +#[cfg(feature = "host")] impl ArtifactPaths { fn artifact_by_name(&self, name: &str) -> PathBuf { self.dir.join(sanitize_artifact_name(name)) } } +#[cfg(feature = "host")] impl PathsAccess for ArtifactPaths { fn lock_file(&self) -> PathBuf { self.dir.join(".lock") } } +#[cfg(feature = "host")] impl ArtifactStore { pub fn new(project_root_locate: Arc) -> Self { Self { @@ -131,6 +143,7 @@ impl ArtifactStore { } #[async_trait] +#[cfg(feature = "host")] impl Access for ArtifactStore { async fn save(&self, name: &str, wasm: &[u8]) -> Result<(), SaveError> { if let Some(len) = artifact_name_overflow(name) { @@ -140,13 +153,17 @@ impl Access for ArtifactStore { } .fail(); } - self.lock()? + let store_err = |e: &dyn std::fmt::Display| SaveError::SaveStore { + source: StoreCause::new(std::io::Error::other(e.to_string())), + name: name.to_owned(), + }; + self.lock() + .map_err(|e| store_err(&e))? .with_write(async |store| { - // Save artifact - write(&store.artifact_by_name(name), wasm).context(SaveWriteFileSnafu)?; - Ok(()) + write(&store.artifact_by_name(name), wasm).map_err(|e| store_err(&e)) }) - .await? + .await + .map_err(|e| store_err(&e))? } async fn lookup(&self, name: &str) -> Result, LookupArtifactError> { @@ -157,7 +174,12 @@ impl Access for ArtifactStore { } .fail(); } - self.lock()? + let store_err = |e: &dyn std::fmt::Display| LookupArtifactError::LookupStore { + source: StoreCause::new(std::io::Error::other(e.to_string())), + name: name.to_owned(), + }; + self.lock() + .map_err(|e| store_err(&e))? .with_read(async |store| { let artifact = store.artifact_by_name(name); // Not Found @@ -168,12 +190,10 @@ impl Access for ArtifactStore { .fail(); } - // Load artifact - let wasm = read(&artifact).context(LookupReadFileSnafu)?; - - Ok(wasm) + read(&artifact).map_err(|e| store_err(&e)) }) - .await? + .await + .map_err(|e| store_err(&e))? } } diff --git a/crates/icp-project/src/store_id.rs b/crates/icp-project/src/store_id.rs index acf74f888..b0f1508c8 100644 --- a/crates/icp-project/src/store_id.rs +++ b/crates/icp-project/src/store_id.rs @@ -1,20 +1,26 @@ use std::collections::BTreeMap; -use std::sync::Arc; -use std::{io::ErrorKind, sync::Mutex}; use ic_agent::export::Principal; -use snafu::{ResultExt, Snafu}; +use snafu::Snafu; +#[cfg(feature = "host")] use crate::{ CACHE_DIR, DATA_DIR, ICP_BASE, fs::{create_dir_all, json, remove_file}, +}; +use crate::{ manifest::{ProjectRootLocate, ProjectRootLocateError}, prelude::*, }; +#[cfg(feature = "host")] +use std::sync::Arc; +#[cfg(feature = "host")] +use std::{io::ErrorKind, sync::Mutex}; /// Mapping of canister names to their Principals within an environment. pub type IdMapping = BTreeMap; +#[cfg(feature = "host")] /// Loads the ID mapping from a given file path. /// /// If the file does not exist, returns an empty mapping. @@ -72,15 +78,6 @@ pub enum RegisterError { #[snafu(transparent)] ProjectRootLocate { source: ProjectRootLocateError }, - #[snafu(display("failed to create directory for canister id store at '{path}'"))] - CreateDirAll { - source: crate::fs::IoError, - path: PathBuf, - }, - - #[snafu(display("failed to load canister id store for environment '{env}'"))] - RegisterLoadStore { source: json::Error, env: String }, - #[snafu(display( "canister '{canister_name}' in environment '{env}' is already registered with id '{id}'", ))] @@ -90,8 +87,11 @@ pub enum RegisterError { id: Principal, }, - #[snafu(display("failed to save canister id mapping for environment '{env}'"))] - RegisterSaveStore { source: json::Error, env: String }, + /// The store could not be read or written. What a store is made of is the + /// implementation's business, so the cause is carried whole — but which + /// environment's store it was is what a reader needs, so that stays. + #[snafu(display("failed to record a canister id for environment '{env}'"))] + RegisterStore { source: StoreCause, env: String }, } #[derive(Debug, Snafu)] @@ -99,11 +99,9 @@ pub enum UnregisterError { #[snafu(transparent)] ProjectRootLocate { source: ProjectRootLocateError }, - #[snafu(display("failed to load canister id store for environment '{env}'"))] - UnregisterLoadStore { source: json::Error, env: String }, - - #[snafu(display("failed to save canister id mapping for environment '{env}'"))] - UnregisterSaveStore { source: json::Error, env: String }, + /// As [`RegisterError::RegisterStore`]. + #[snafu(display("failed to remove a canister id from environment '{env}'"))] + UnregisterStore { source: StoreCause, env: String }, } #[derive(Debug, Snafu)] @@ -111,8 +109,9 @@ pub enum LookupIdError { #[snafu(transparent)] ProjectRootLocate { source: ProjectRootLocateError }, - #[snafu(display("failed to load canister id store for environment '{env}'"))] - LookupLoadStore { source: json::Error, env: String }, + /// As [`RegisterError::RegisterStore`]. + #[snafu(display("failed to read the canister id store for environment '{env}'"))] + LookupStore { source: StoreCause, env: String }, #[snafu(display("could not find ID for canister '{canister_name}' in environment '{env}'"))] IdNotFound { env: String, canister_name: String }, @@ -126,18 +125,47 @@ pub enum CleanupError { #[snafu(transparent)] ProjectRootLocate { source: ProjectRootLocateError }, - #[snafu(transparent)] - DeleteFile { source: crate::fs::IoError }, + /// As [`RegisterError::RegisterStore`]. + #[snafu(display("failed to clear the canister id store for environment '{env}'"))] + CleanupStore { source: StoreCause, env: String }, +} + +/// Whatever went wrong inside a store implementation. +/// +/// Opaque on purpose: the trait says nothing about what a store is made of, so +/// this carries the cause without naming it. It displays and chains as itself, +/// so nothing is lost from what the user is told. +#[derive(Debug)] +pub struct StoreCause(Box); + +impl StoreCause { + pub fn new(source: impl std::error::Error + Send + Sync + 'static) -> Self { + Self(Box::new(source)) + } +} + +impl std::fmt::Display for StoreCause { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +impl std::error::Error for StoreCause { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.0.source() + } } /// Store of canister ID mappings for environments. /// /// Each environment has a separate file storing its canister IDs mapping. +#[cfg(feature = "host")] pub struct AccessImpl { project_root_locate: Arc, lock: Mutex<()>, } +#[cfg(feature = "host")] impl AccessImpl { pub fn new(project_root_locate: Arc) -> Self { Self { @@ -147,6 +175,7 @@ impl AccessImpl { } } +#[cfg(feature = "host")] impl Access for AccessImpl { fn register( &self, @@ -159,12 +188,14 @@ impl Access for AccessImpl { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; - create_dir_all(fpath.parent().unwrap()).context(CreateDirAllSnafu { - path: fpath.clone(), + create_dir_all(fpath.parent().unwrap()).map_err(|e| RegisterError::RegisterStore { + source: StoreCause::new(e), + env: env.to_owned(), })?; // Load the file - let mut mapping = load_mapping(&fpath).context(RegisterLoadStoreSnafu { + let mut mapping = load_mapping(&fpath).map_err(|e| RegisterError::RegisterStore { + source: StoreCause::new(e), env: env.to_owned(), })?; @@ -179,7 +210,8 @@ impl Access for AccessImpl { } // Store JSON - json::save(&fpath, &mapping).context(RegisterSaveStoreSnafu { + json::save(&fpath, &mapping).map_err(|e| RegisterError::RegisterStore { + source: StoreCause::new(e), env: env.to_owned(), })?; @@ -198,7 +230,8 @@ impl Access for AccessImpl { let fpath = self.get_fpath_for_env(is_cache, env)?; // Load the file - let mut mapping = load_mapping(&fpath).context(UnregisterLoadStoreSnafu { + let mut mapping = load_mapping(&fpath).map_err(|e| UnregisterError::UnregisterStore { + source: StoreCause::new(e), env: env.to_owned(), })?; @@ -206,7 +239,8 @@ impl Access for AccessImpl { mapping.remove(canister_name); // Store JSON - json::save(&fpath, &mapping).context(UnregisterSaveStoreSnafu { + json::save(&fpath, &mapping).map_err(|e| UnregisterError::UnregisterStore { + source: StoreCause::new(e), env: env.to_owned(), })?; @@ -222,7 +256,8 @@ impl Access for AccessImpl { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; load_mapping(&fpath) - .context(LookupLoadStoreSnafu { + .map_err(|e| LookupIdError::LookupStore { + source: StoreCause::new(e), env: env.to_owned(), })? .get(canister_name) @@ -236,7 +271,8 @@ impl Access for AccessImpl { fn lookup_by_environment(&self, is_cache: bool, env: &str) -> Result { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; - load_mapping(&fpath).context(LookupLoadStoreSnafu { + load_mapping(&fpath).map_err(|e| LookupIdError::LookupStore { + source: StoreCause::new(e), env: env.to_owned(), }) } @@ -245,12 +281,16 @@ impl Access for AccessImpl { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; if fpath.exists() { - remove_file(&fpath)?; + remove_file(&fpath).map_err(|e| CleanupError::CleanupStore { + source: StoreCause::new(e), + env: env.to_owned(), + })?; } Ok(()) } } +#[cfg(feature = "host")] impl AccessImpl { /// Gets the ID mapping file path for a given environment. /// From 5b400541aa41cdb09a79b284ff6c6cc66c0f0280 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 05:53:02 -0700 Subject: [PATCH 02/12] fix: pass the filesystem seam's cause through instead of restating it `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`. --- crates/icp-project/src/files.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 71e800407..23af3908b 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -23,7 +23,7 @@ use crate::prelude::*; /// displayed as itself. Implementations name the path in their own error, which /// is what a reader needs to see. #[derive(Debug, Snafu)] -#[snafu(display("{source}"))] +#[snafu(transparent)] pub struct FsError { pub source: Box, } From aa7ccf621fc28d580f4bbed5da350dd6ce0ac6b6 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:35:48 -0700 Subject: [PATCH 03/12] fix: resolve `..` and absolute glob patterns as joining them would MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/icp-project/src/files.rs | 134 +++++++++++++++++++++++++------- 1 file changed, 104 insertions(+), 30 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 23af3908b..f62c72fe8 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -12,6 +12,7 @@ //! not reach for it. use async_trait::async_trait; +use camino::Utf8Component; use snafu::{ResultExt, Snafu}; use crate::prelude::*; @@ -145,6 +146,11 @@ pub enum GlobError { /// `**` standing for any number of directories — the same shape `glob` /// supports, and the same one the manifest reference documents. /// +/// A component that cannot match anything is instead resolved the way joining +/// it onto `base` would resolve it: `..` climbs, and an absolute pattern starts +/// from its own root with `base` dropped. So a pattern with no metacharacters +/// at all names the same path here as `base.join(pattern)` does. +/// /// Only paths that exist are returned, and each directory's entries are listed /// in sorted order, so the result is the same on every run. pub async fn expand_glob( @@ -154,45 +160,70 @@ pub async fn expand_glob( ) -> Result, GlobError> { let mut frontier = vec![base.to_path_buf()]; - for component in pattern.split('/') { - if component.is_empty() || component == "." { - continue; - } + for component in Path::new(pattern).components() { + match component { + // A root — or, on Windows, a drive prefix — is what makes a pattern + // absolute. Pushing it is what leaves `base` behind, by the same + // rule that joining an absolute path onto another discards the + // other. + Utf8Component::Prefix(_) | Utf8Component::RootDir => { + for dir in &mut frontier { + dir.push(component.as_str()); + } + } + + Utf8Component::CurDir => {} + + // No listing ever turns up an entry named `..`, so this names a + // directory rather than matching one. It still has to be one, or + // the pattern describes no path from here. + Utf8Component::ParentDir => { + let mut next = Vec::new(); + for dir in &frontier { + let parent = dir.join(".."); + if files.is_dir(&parent).await { + next.push(parent); + } + } + frontier = next; + } - if component == "**" { // `**` matches zero or more directories, so every reachable // directory — including the ones already in hand — carries forward. - let mut reached = frontier.clone(); - let mut stack = frontier; - while let Some(dir) = stack.pop() { - for entry in list(files, &dir, pattern).await? { - if files.is_dir(&entry).await { - reached.push(entry.clone()); - stack.push(entry); + Utf8Component::Normal("**") => { + let mut reached = frontier.clone(); + let mut stack = frontier; + while let Some(dir) = stack.pop() { + for entry in list(files, &dir, pattern).await? { + if files.is_dir(&entry).await { + reached.push(entry.clone()); + stack.push(entry); + } } } + reached.sort(); + reached.dedup(); + frontier = reached; } - reached.sort(); - reached.dedup(); - frontier = reached; - continue; - } - - let matcher = glob::Pattern::new(component).context(PatternSnafu { - pattern: pattern.to_owned(), - })?; - let mut next = Vec::new(); - for dir in &frontier { - for entry in list(files, dir, pattern).await? { - if entry.file_name().is_some_and(|name| matcher.matches(name)) { - next.push(entry); + Utf8Component::Normal(component) => { + let matcher = glob::Pattern::new(component).context(PatternSnafu { + pattern: pattern.to_owned(), + })?; + + let mut next = Vec::new(); + for dir in &frontier { + for entry in list(files, dir, pattern).await? { + if entry.file_name().is_some_and(|name| matcher.matches(name)) { + next.push(entry); + } + } } + next.sort(); + next.dedup(); + frontier = next; } } - next.sort(); - next.dedup(); - frontier = next; } Ok(frontier) @@ -232,7 +263,13 @@ mod tests { } async fn expand(root: &Path, pattern: &str) -> Vec { - expand_glob(&HostFileSystem, root, pattern) + expand_from(root, root, pattern).await + } + + /// As [`expand`], but expanding from a `base` the pattern may leave: results + /// are still spelled relative to `root`. + async fn expand_from(base: &Path, root: &Path, pattern: &str) -> Vec { + expand_glob(&HostFileSystem, base, pattern) .await .expect("expand") .into_iter() @@ -312,6 +349,43 @@ mod tests { assert_eq!(expand(d.path(), "c/*").await, ["c/a", "c/m", "c/z"]); } + /// A dependency next to the project, rather than under it, is named by + /// climbing out of it — the shape a workspace of sibling projects uses. + #[tokio::test] + async fn a_parent_component_climbs_out_of_the_base() { + let d = tree(&[ + "proj/icp.yaml", + "shared/a/canister.yaml", + "shared/b/canister.yaml", + ]); + assert_eq!( + expand_from(&d.path().join("proj"), d.path(), "../shared/*").await, + ["proj/../shared/a", "proj/../shared/b"] + ); + } + + /// `..` has to name a directory that is there, like any other component. + #[tokio::test] + async fn climbing_to_nowhere_matches_nothing() { + let d = tree(&["proj/icp.yaml"]); + assert!( + expand_from(&d.path().join("proj/nonexistent"), d.path(), "../*") + .await + .is_empty() + ); + } + + #[tokio::test] + async fn an_absolute_pattern_leaves_the_base_behind() { + let d = tree(&["shared/a/canister.yaml", "shared/b/canister.yaml"]); + let elsewhere = tree(&["unrelated/canister.yaml"]); + let pattern = format!("{}/*", d.path().join("shared")); + assert_eq!( + expand_from(elsewhere.path(), d.path(), &pattern).await, + ["shared/a", "shared/b"] + ); + } + #[tokio::test] async fn a_malformed_pattern_is_reported_as_such() { let d = tree(&["c/a"]); From cffb2191c5778f2033fed181a1e83369f2c9e055 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:36:33 -0700 Subject: [PATCH 04/12] fix: compile the hostless build that exposes the mocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--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. --- crates/icp-project/src/files.rs | 48 ++++++++++++++++++++++++ crates/icp-project/src/host.rs | 2 +- crates/icp-project/src/manifest/mod.rs | 1 + crates/icp-project/src/store_artifact.rs | 6 ++- crates/icp-project/src/store_id.rs | 9 +++-- 5 files changed, 60 insertions(+), 6 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index f62c72fe8..5c7b3dc58 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -122,6 +122,54 @@ impl FileSystem for HostFileSystem { } } +#[cfg(any(test, feature = "test-util"))] +/// A [`FileSystem`] for tests on paths that never reach a file. +pub struct UnimplementedMockFileSystem; + +#[cfg(any(test, feature = "test-util"))] +#[async_trait] +impl FileSystem for UnimplementedMockFileSystem { + async fn read(&self, _path: &Path) -> Result, FsError> { + unimplemented!("UnimplementedMockFileSystem::read") + } + + async fn read_to_string(&self, _path: &Path) -> Result { + unimplemented!("UnimplementedMockFileSystem::read_to_string") + } + + async fn write(&self, _path: &Path, _contents: &[u8]) -> Result<(), FsError> { + unimplemented!("UnimplementedMockFileSystem::write") + } + + async fn create_dir_all(&self, _path: &Path) -> Result<(), FsError> { + unimplemented!("UnimplementedMockFileSystem::create_dir_all") + } + + async fn copy(&self, _from: &Path, _to: &Path) -> Result<(), FsError> { + unimplemented!("UnimplementedMockFileSystem::copy") + } + + async fn exists(&self, _path: &Path) -> bool { + unimplemented!("UnimplementedMockFileSystem::exists") + } + + async fn is_file(&self, _path: &Path) -> bool { + unimplemented!("UnimplementedMockFileSystem::is_file") + } + + async fn is_dir(&self, _path: &Path) -> bool { + unimplemented!("UnimplementedMockFileSystem::is_dir") + } + + async fn read_dir(&self, _path: &Path) -> Result, FsError> { + unimplemented!("UnimplementedMockFileSystem::read_dir") + } + + async fn canonicalize(&self, _path: &Path) -> Option { + unimplemented!("UnimplementedMockFileSystem::canonicalize") + } +} + /// A glob pattern could not be understood. #[derive(Debug, Snafu)] pub enum GlobError { diff --git a/crates/icp-project/src/host.rs b/crates/icp-project/src/host.rs index 53255da21..a432a0392 100644 --- a/crates/icp-project/src/host.rs +++ b/crates/icp-project/src/host.rs @@ -110,7 +110,7 @@ impl Host { pub fn mocked() -> Self { Self { project: Arc::new(crate::MockProjectLoader::minimal()), - files: Arc::new(crate::files::HostFileSystem), + files: Arc::new(crate::files::UnimplementedMockFileSystem), ids: Arc::new(crate::store_id::mock::MockInMemoryIdStore::new()), artifacts: Arc::new(crate::store_artifact::MockInMemoryArtifactStore::new()), builder: Arc::new(crate::canister::build::UnimplementedMockBuilder), diff --git a/crates/icp-project/src/manifest/mod.rs b/crates/icp-project/src/manifest/mod.rs index 3172b0909..68e7ed7b1 100644 --- a/crates/icp-project/src/manifest/mod.rs +++ b/crates/icp-project/src/manifest/mod.rs @@ -1,3 +1,4 @@ +#[cfg(feature = "host")] use std::collections::HashSet; use std::marker::PhantomData; diff --git a/crates/icp-project/src/store_artifact.rs b/crates/icp-project/src/store_artifact.rs index 5bd835aac..9fdb5614c 100644 --- a/crates/icp-project/src/store_artifact.rs +++ b/crates/icp-project/src/store_artifact.rs @@ -1,3 +1,4 @@ +#[cfg(feature = "host")] use std::sync::Arc; #[cfg(any(test, feature = "test-util"))] use std::{collections::HashMap, sync::Mutex}; @@ -9,8 +10,10 @@ use crate::{ lock::{DirectoryStructureLock, PathsAccess}, read, write, }, + manifest::ProjectRootLocate, + prelude::*, + store_id::StoreCause, }; -use crate::{manifest::ProjectRootLocate, prelude::*, store_id::StoreCause}; use async_trait::async_trait; use snafu::Snafu; @@ -100,7 +103,6 @@ const NAME_MAX: usize = 255; /// nested dependency store key can stay within the total path limit yet blow the /// per-component limit once its separators are percent-encoded. #[cfg(feature = "host")] -#[cfg(feature = "host")] fn artifact_name_overflow(name: &str) -> Option { let len = sanitize_artifact_name(name).len(); (len > NAME_MAX).then_some(len) diff --git a/crates/icp-project/src/store_id.rs b/crates/icp-project/src/store_id.rs index b0f1508c8..100a45331 100644 --- a/crates/icp-project/src/store_id.rs +++ b/crates/icp-project/src/store_id.rs @@ -3,13 +3,12 @@ use std::collections::BTreeMap; use ic_agent::export::Principal; use snafu::Snafu; +use crate::manifest::ProjectRootLocateError; #[cfg(feature = "host")] use crate::{ CACHE_DIR, DATA_DIR, ICP_BASE, fs::{create_dir_all, json, remove_file}, -}; -use crate::{ - manifest::{ProjectRootLocate, ProjectRootLocateError}, + manifest::ProjectRootLocate, prelude::*, }; #[cfg(feature = "host")] @@ -315,6 +314,10 @@ impl AccessImpl { #[cfg(any(test, feature = "test-util"))] pub mod mock { use super::*; + // Not from `super`: its `Mutex` belongs to the host implementation, which a + // hostless build leaves out while still exposing these mocks. + use std::sync::Mutex; + /// In-memory mock implementation of `Access`. /// /// There are two separate stores for cache and data, to allow testing both paths. From c04339af226233ef076ae0e7e0e5d079213a1b60 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:36:54 -0700 Subject: [PATCH 05/12] fix: carry the artifact store's cause whole instead of stringifying it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/icp-project/src/store_artifact.rs | 46 ++++++++++++++++-------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/crates/icp-project/src/store_artifact.rs b/crates/icp-project/src/store_artifact.rs index 9fdb5614c..402fadc3c 100644 --- a/crates/icp-project/src/store_artifact.rs +++ b/crates/icp-project/src/store_artifact.rs @@ -144,6 +144,32 @@ impl ArtifactStore { } } +/// Carries what went wrong inside the store into [`SaveError::SaveStore`], +/// whole: the cause's own chain is what says which file it was and why it +/// failed, and this layer has nothing to add to it. +/// +/// A free function rather than a closure because the store's several steps fail +/// in their own types — a lock, a write — and each is carried as itself. +#[cfg(feature = "host")] +fn save_store(name: &str, source: impl std::error::Error + Send + Sync + 'static) -> SaveError { + SaveError::SaveStore { + source: StoreCause::new(source), + name: name.to_owned(), + } +} + +/// As [`save_store`], for [`LookupArtifactError::LookupStore`]. +#[cfg(feature = "host")] +fn lookup_store( + name: &str, + source: impl std::error::Error + Send + Sync + 'static, +) -> LookupArtifactError { + LookupArtifactError::LookupStore { + source: StoreCause::new(source), + name: name.to_owned(), + } +} + #[async_trait] #[cfg(feature = "host")] impl Access for ArtifactStore { @@ -155,17 +181,13 @@ impl Access for ArtifactStore { } .fail(); } - let store_err = |e: &dyn std::fmt::Display| SaveError::SaveStore { - source: StoreCause::new(std::io::Error::other(e.to_string())), - name: name.to_owned(), - }; self.lock() - .map_err(|e| store_err(&e))? + .map_err(|e| save_store(name, e))? .with_write(async |store| { - write(&store.artifact_by_name(name), wasm).map_err(|e| store_err(&e)) + write(&store.artifact_by_name(name), wasm).map_err(|e| save_store(name, e)) }) .await - .map_err(|e| store_err(&e))? + .map_err(|e| save_store(name, e))? } async fn lookup(&self, name: &str) -> Result, LookupArtifactError> { @@ -176,12 +198,8 @@ impl Access for ArtifactStore { } .fail(); } - let store_err = |e: &dyn std::fmt::Display| LookupArtifactError::LookupStore { - source: StoreCause::new(std::io::Error::other(e.to_string())), - name: name.to_owned(), - }; self.lock() - .map_err(|e| store_err(&e))? + .map_err(|e| lookup_store(name, e))? .with_read(async |store| { let artifact = store.artifact_by_name(name); // Not Found @@ -192,10 +210,10 @@ impl Access for ArtifactStore { .fail(); } - read(&artifact).map_err(|e| store_err(&e)) + read(&artifact).map_err(|e| lookup_store(name, e)) }) .await - .map_err(|e| store_err(&e))? + .map_err(|e| lookup_store(name, e))? } } From 337881dab3dbc1289cf08383d308dbafd2db2ec2 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:49:34 -0700 Subject: [PATCH 06/12] fix: resolve a pruned reference through the same seam that keyed it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 `:` 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. --- crates/icp-project/src/operations/bundle.rs | 79 +++++++++++++++++---- 1 file changed, 66 insertions(+), 13 deletions(-) diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 902346eda..3b093b3e0 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -341,9 +341,14 @@ struct Pruned<'a> { /// Store keys of the canisters the environment does not hold. dropped: &'a HashSet, - /// Each workspace instance's store-key prefix, by its canonical directory. + /// Each workspace instance's store-key prefix, by its canonical directory + /// as [`FileSystem::canonicalize`] spells one. prefixes_by_dir: &'a HashMap, + /// Where the directories those keys name are canonicalized, so a lookup + /// spells a directory the same way the key does. + files: &'a dyn FileSystem, + /// The environment the bundle is built for, for diagnostics. environment: &'a str, } @@ -354,8 +359,9 @@ impl Pruned<'_> { /// /// A name that resolves to no instance in the workspace is left alone: it is /// invalid, and reporting it is the manifest loader's job, not the bundler's. - fn drops(&self, instance: &Instance, name: &str) -> bool { + async fn drops(&self, instance: &Instance, name: &str) -> bool { self.store_key(instance, name) + .await .is_some_and(|key| self.dropped.contains(&key)) } @@ -364,11 +370,11 @@ impl Pruned<'_> { /// canister of a project that instance reaches through its dependencies. The /// path is the one the store key's own prefix is built from, so resolving it /// against the instance's directory gives that prefix back. - fn store_key(&self, instance: &Instance, name: &str) -> Option { + async fn store_key(&self, instance: &Instance, name: &str) -> Option { let Some((rel, local)) = name.rsplit_once(':') else { return Some(override_store_key(&instance.prefix, name)); }; - let dir = instance.dir.join(rel).canonicalize_utf8().ok()?; + let dir = self.files.canonicalize(&instance.dir.join(rel)).await?; Some(override_store_key(self.prefixes_by_dir.get(&dir)?, local)) } } @@ -417,6 +423,7 @@ pub async fn create_bundle( let pruned = Pruned { dropped: &dropped, prefixes_by_dir: &prefixes_by_dir, + files, environment, }; let canonical_project_dir = canonicalize(files, project_dir).await?; @@ -1138,7 +1145,7 @@ async fn inline_environments( // override for a left-out canister resolves its paths against that // canister's directory, which the bundle no longer knows. if let Item::Manifest(ref mut env) = inlined { - prune_environment(env, instance, pruned); + prune_environment(env, instance, pruned).await; } if let Item::Manifest(ref mut env) = inlined { @@ -1216,12 +1223,26 @@ async fn inline_environments( /// The environment being pruned is not necessarily the one the bundle was built /// for — a bundle keeps every environment its manifests declare, and each of /// them can only ever hold canisters the bundle carries. -fn prune_environment(env: &mut EnvironmentManifest, instance: &Instance, pruned: &Pruned<'_>) { - env.canisters = prune_selection(std::mem::take(&mut env.canisters), |name| { - pruned.drops(instance, name) - }); +async fn prune_environment( + env: &mut EnvironmentManifest, + instance: &Instance, + pruned: &Pruned<'_>, +) { + // Resolving a name written as `:` reaches the filesystem, so + // every name this environment mentions is put to `drops` up front and the + // passes below are lookups. Whatever they ask about, `mentioned_canisters` + // has to have gathered. + let mut dropped = HashSet::new(); + for name in mentioned_canisters(env) { + if pruned.drops(instance, &name).await { + dropped.insert(name); + } + } + let drops = |name: &str| dropped.contains(name); + + env.canisters = prune_selection(std::mem::take(&mut env.canisters), drops); if let Some(settings) = &mut env.settings { - settings.retain(|name, _| !pruned.drops(instance, name)); + settings.retain(|name, _| !drops(name)); // An override's own controller list survives the pruning above, which // only reaches the canister an override configures: a kept canister can // still be handed a controller the bundle does not carry. @@ -1230,7 +1251,7 @@ fn prune_environment(env: &mut EnvironmentManifest, instance: &Instance, pruned: continue; }; controllers.retain(|cref| match cref { - ControllerRef::CanisterName(name) if pruned.drops(instance, name) => { + ControllerRef::CanisterName(name) if drops(name) => { warn!( "Environment '{}' names '{name}' as a controller of '{canister}', which \ environment '{}' does not contain; the bundle drops the reference.", @@ -1243,11 +1264,43 @@ fn prune_environment(env: &mut EnvironmentManifest, instance: &Instance, pruned: } } if let Some(init_args) = &mut env.init_args { - init_args.retain(|name, _| !pruned.drops(instance, name)); + init_args.retain(|name, _| !drops(name)); } if let Some(upgrade_args) = &mut env.upgrade_args { - upgrade_args.retain(|name, _| !pruned.drops(instance, name)); + upgrade_args.retain(|name, _| !drops(name)); + } +} + +/// Every canister name an environment mentions: the canisters it selects, the +/// canisters whose settings or args it overrides, and the ones those settings +/// name as controllers. +/// +/// Kept beside [`prune_environment`], which asks about each of them and must not +/// ask about one this misses. +fn mentioned_canisters(env: &EnvironmentManifest) -> HashSet { + let mut names = HashSet::new(); + if let CanisterSelection::Named(selected) = &env.canisters { + names.extend(selected.iter().cloned()); + } + if let Some(settings) = &env.settings { + names.extend(settings.keys().cloned()); + for overrides in settings.values() { + let Some(controllers) = &overrides.controllers else { + continue; + }; + names.extend(controllers.iter().filter_map(|cref| match cref { + ControllerRef::CanisterName(name) => Some(name.clone()), + ControllerRef::Principal(_) => None, + })); + } + } + for overrides in [env.init_args.as_ref(), env.upgrade_args.as_ref()] + .into_iter() + .flatten() + { + names.extend(overrides.keys().cloned()); } + names } /// Load `icp_appmanifest.yaml` if present, rewriting its top-level `images` paths to point at From 16c8f62b262bf4733b71050777a713dfa7b4f30e Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:49:57 -0700 Subject: [PATCH 07/12] fix: take the build's scratch directory from the seam it reads through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/icp-project/src/files.rs | 34 ++++++++++++++++++++++ crates/icp-project/src/operations/build.rs | 8 +++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 5c7b3dc58..bdb8dd3b7 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -71,6 +71,19 @@ pub trait FileSystem: Send + Sync { /// Resolve `..` and symlinks. `None` when the path does not resolve. async fn canonicalize(&self, path: &Path) -> Option; + + /// Somewhere to put files that only one caller needs and nothing keeps. + /// + /// A build step writes its module to a path handed to it and the operation + /// reads it back, so the two have to meet somewhere — and it is this + /// implementation, not the operation, that knows where a path it can read + /// is allowed to come from. + async fn scratch_dir(&self) -> Result, FsError>; +} + +/// A directory that exists for as long as this is held, and is removed with it. +pub trait Scratch: Send + Sync { + fn path(&self) -> &Path; } #[cfg(feature = "host")] @@ -120,6 +133,23 @@ impl FileSystem for HostFileSystem { async fn canonicalize(&self, path: &Path) -> Option { PathBuf::from_path_buf(dunce::canonicalize(path).ok()?).ok() } + + async fn scratch_dir(&self) -> Result, FsError> { + let dir = camino_tempfile::tempdir().map_err(FsError::new)?; + Ok(Box::new(HostScratch(dir))) + } +} + +/// A temporary directory of this machine's, which `tempfile` removes when the +/// [`Utf8TempDir`](camino_tempfile::Utf8TempDir) drops. +#[cfg(feature = "host")] +struct HostScratch(camino_tempfile::Utf8TempDir); + +#[cfg(feature = "host")] +impl Scratch for HostScratch { + fn path(&self) -> &Path { + self.0.path() + } } #[cfg(any(test, feature = "test-util"))] @@ -168,6 +198,10 @@ impl FileSystem for UnimplementedMockFileSystem { async fn canonicalize(&self, _path: &Path) -> Option { unimplemented!("UnimplementedMockFileSystem::canonicalize") } + + async fn scratch_dir(&self) -> Result, FsError> { + unimplemented!("UnimplementedMockFileSystem::scratch_dir") + } } /// A glob pattern could not be understood. diff --git a/crates/icp-project/src/operations/build.rs b/crates/icp-project/src/operations/build.rs index cd85520ae..054de2ac2 100644 --- a/crates/icp-project/src/operations/build.rs +++ b/crates/icp-project/src/operations/build.rs @@ -5,7 +5,6 @@ use crate::{ canister::build::{Build, BuildError, Params}, prelude::*, }; -use camino_tempfile::tempdir; use futures::{StreamExt, stream::FuturesOrdered}; use icp_events::{StepOutcome, TaskOutcome}; @@ -15,7 +14,7 @@ use snafu::{ResultExt, Snafu}; #[derive(Debug, Snafu)] pub enum BuildOperationError { #[snafu(display("failed to create temporary build directory"))] - TempDir { source: std::io::Error }, + TempDir { source: crate::files::FsError }, #[snafu(transparent)] Build { source: BuildError }, @@ -47,7 +46,10 @@ pub async fn build( artifacts: Arc, files: &dyn crate::files::FileSystem, ) -> Result<(), BuildOperationError> { - let build_dir = tempdir().context(TempDirSnafu)?; + // From `files`, not from this machine: the step writes the module there and + // the read below comes back through the same seam, so both have to be + // looking at the same directory. + let build_dir = files.scratch_dir().await.context(TempDirSnafu)?; let wasm_output_path = build_dir.path().join("out.wasm"); let step_count = canister.build.steps.len(); From 6ee83d3775b72c4a96e4556962a383343f8febcd Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:50:19 -0700 Subject: [PATCH 08/12] test: name the literal-component test after what it checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/icp-project/src/files.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index bdb8dd3b7..f93863396 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -373,13 +373,20 @@ mod tests { ); } + /// A literal component is still matched by listing its parent, so it yields + /// the one path it names — and nothing when that path is not there. #[tokio::test] - async fn a_literal_component_needs_no_listing() { + async fn literal_components_name_one_path() { let d = tree(&["canisters/a/canister.yaml"]); assert_eq!( expand(d.path(), "canisters/a/canister.yaml").await, ["canisters/a/canister.yaml"] ); + assert!( + expand(d.path(), "canisters/a/nothing.yaml") + .await + .is_empty() + ); } /// `**` stands for zero or more directories, so a pattern that uses it also From c80e35acd0e0f2b1d433e27bedcb6a3843c79291 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 06:54:39 -0700 Subject: [PATCH 09/12] fix: ask the filesystem seam whether a manifest is there Consolidation reads every manifest through `FileSystem`, but six of the checks guarding those reads still called `Utf8Path::is_file`. A project whose files are not this machine's would report each canister, dependency, network and environment it names as missing, and never reach the read that would have found it. The glob branch beside the first of them already asked `files`; the explicit-path branch next to it did not. The two that asked `exists()` first now ask only `is_file`: a path that is a file exists. --- crates/icp-project/src/project.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index 954a6fb82..8f6085693 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -377,7 +377,7 @@ async fn build_manifest_canisters( // For explicit paths, validate that they exist and contain canister.yaml let mut validated_paths = vec![]; for p in paths { - if !p.join(CANISTER_MANIFEST).is_file() { + if !files.is_file(&p.join(CANISTER_MANIFEST)).await { return NotFoundSnafu { kind: "canister".to_string(), path: pattern.to_string(), @@ -707,7 +707,7 @@ async fn resolve_edges( let mut out = Vec::with_capacity(manifest.dependencies.len()); for dep in &manifest.dependencies { let dep_root = dir.join(&dep.path); - if !dep_root.join(PROJECT_MANIFEST).is_file() { + if !files.is_file(&dep_root.join(PROJECT_MANIFEST)).await { return InstanceNotFoundSnafu { alias: dep.name.clone(), path: dep_root, @@ -942,7 +942,7 @@ async fn import_dependency( ) -> Result { let dep_root = parent_dir.join(&dep.path); let manifest_path = dep_root.join(PROJECT_MANIFEST); - if !manifest_path.is_file() { + if !files.is_file(&manifest_path).await { return DependencyNotFoundSnafu { alias: dep.name.clone(), path: dep_root.to_string(), @@ -1042,7 +1042,7 @@ async fn import_dependency( Item::Manifest(m) => m.clone(), Item::Path(path) => { let p = dep_root.join(path); - if !p.is_file() { + if !files.is_file(&p).await { return NotFoundSnafu { kind: "environment".to_string(), path: p.to_string(), @@ -1438,7 +1438,7 @@ pub async fn consolidate_manifest( let m = match i { Item::Path(path) => { let path = pdir.join(path); - if !path.exists() || !path.is_file() { + if !files.is_file(&path).await { return NotFoundSnafu { kind: "network".to_string(), path: path.to_string(), @@ -1518,7 +1518,7 @@ pub async fn consolidate_manifest( let m = match i { Item::Path(path) => { let path = pdir.join(path); - if !path.exists() || !path.is_file() { + if !files.is_file(&path).await { return NotFoundSnafu { kind: "environment".to_string(), path: path.to_string(), From a6b60adcdfc4483f79cbdb2decfd0a62cb204aef Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 06:54:48 -0700 Subject: [PATCH 10/12] fix: walk each directory once when expanding `**` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_dir` answers through symlinks, so a directory that links back to an ancestor is one directory reachable under endlessly many paths, and `**` descended every one of them. On macOS the walk stopped only when the kernel refused a seventeenth link, having matched the same file sixteen times under ever longer names; an implementation with no such limit would not have stopped at all. Descending is keyed on `canonicalize` now, so the first spelling of a directory is the one that matches and a later one matches nothing — the files under it are the same files, and reporting them twice would make one canister look like two. An implementation that cannot establish identity is descended anyway: it has no links to come back around, or it would be able to resolve them. --- crates/icp-project/src/files.rs | 49 +++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index f93863396..9f3618c6b 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -11,6 +11,8 @@ //! wrappers over `std::fs` whose errors carry the path. Project code should //! not reach for it. +use std::collections::HashSet; + use async_trait::async_trait; use camino::Utf8Component; use snafu::{ResultExt, Snafu}; @@ -234,7 +236,9 @@ pub enum GlobError { /// at all names the same path here as `base.join(pattern)` does. /// /// Only paths that exist are returned, and each directory's entries are listed -/// in sorted order, so the result is the same on every run. +/// in sorted order, so the result is the same on every run. `**` descends into +/// each directory once, identified rather than spelled, so a symlink cycle +/// terminates. pub async fn expand_glob( files: &dyn FileSystem, base: &Path, @@ -272,12 +276,39 @@ pub async fn expand_glob( // `**` matches zero or more directories, so every reachable // directory — including the ones already in hand — carries forward. + // + // Descending is keyed on a directory's identity rather than its + // path, because `is_dir` answers through symlinks: a link back to + // an ancestor is one directory reachable under endlessly many + // paths, and descending each of them yields the same files again + // under an ever longer name, until the implementation refuses to + // resolve any more — or, where nothing refuses, never. So the + // first spelling of a directory is the one that matches and is + // descended, and a later one matches nothing at all: the files + // under it are the same files, and reporting them twice would make + // one canister look like two. + // + // An implementation that cannot establish identity is descended + // anyway: it has no links to come back around, or it would be able + // to resolve them. Utf8Component::Normal("**") => { + let mut seen = HashSet::new(); let mut reached = frontier.clone(); + for dir in &reached { + if let Some(id) = files.canonicalize(dir).await { + seen.insert(id); + } + } + let mut stack = frontier; while let Some(dir) = stack.pop() { for entry in list(files, &dir, pattern).await? { - if files.is_dir(&entry).await { + if files.is_dir(&entry).await + && files + .canonicalize(&entry) + .await + .is_none_or(|id| seen.insert(id)) + { reached.push(entry.clone()); stack.push(entry); } @@ -409,6 +440,20 @@ mod tests { ); } + /// A directory that links back to an ancestor is reachable under endlessly + /// many paths. `**` still finishes, and reports each file once. + #[cfg(unix)] + #[tokio::test] + async fn a_double_star_does_not_follow_a_symlink_cycle() { + let d = tree(&["services/a/one.yaml"]); + std::os::unix::fs::symlink(d.path().join("services"), d.path().join("services/a/loop")) + .expect("symlink"); + assert_eq!( + expand(d.path(), "services/**/*.yaml").await, + ["services/a/one.yaml"] + ); + } + #[tokio::test] async fn character_classes_and_question_marks_work() { let d = tree(&["c/a1.yaml", "c/b2.yaml", "c/cc.yaml"]); From b8f99e20448aa068bba6210428b1bcb3416fc134 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 06:54:55 -0700 Subject: [PATCH 11/12] docs: say what the `host` feature gates, and what it does not yet The feature listed subprocesses among what it gates, which it does not: build script steps go straight to `tokio::process`, and `HostScripts` sits in front of the sync ones without a gate. Randomness and the `tar` directory walk are outside it too, and every dependency is still linked either way, so the list now says what is covered and names what is not. `ArchiveWriter::dir` gets the reason it reads the host filesystem rather than the seam the rest of bundling reads through: `tar`'s own walk is what keeps a symlink a symlink, and the seam reports a link as the file it points at with no way to say otherwise. --- crates/icp-project/Cargo.toml | 11 +++++++++-- crates/icp-project/src/operations/bundle.rs | 8 ++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/icp-project/Cargo.toml b/crates/icp-project/Cargo.toml index 3d8d061a9..b24cc8bf5 100644 --- a/crates/icp-project/Cargo.toml +++ b/crates/icp-project/Cargo.toml @@ -8,8 +8,15 @@ publish.workspace = true [features] default = ["host"] # Implementations of this crate's seams that use the machine it is running on: -# the filesystem, the project-local `.icp` stores, subprocesses. Turned off for -# a build that has to run somewhere without them, such as inside a canister. +# the filesystem and the project-local `.icp` stores. Turned off for a build +# that has to run somewhere without them, such as inside a canister. +# +# It does not yet cover everything host-shaped here — the step runners still +# spawn subprocesses and drive a wasmtime sandbox unconditionally, `create` +# draws entropy from `rand`, and a bundle's plugin directories go through +# `tar`'s own directory walk. Each is a seam of its own to draw, and every +# dependency is still non-optional besides, so this is the boundary the feature +# claims rather than a canister target. host = [] # Exposes this crate's mocks and fixtures so downstream crates can test against # the same seams. Off in a normal build, so none of it ships. diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 3b093b3e0..f1b6ed0da 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -1466,6 +1466,14 @@ impl ArchiveWriter { }) } + /// Appends a directory's whole contents, walking the host filesystem rather + /// than the [`FileSystem`] seam the rest of bundling reads through. + /// + /// `tar`'s walk is what keeps a symlink a symlink, per + /// [`new`](ArchiveWriter::new); the seam reports a link's target as the file + /// it points at and has no way to say otherwise, so reading through it would + /// slurp whatever the link reaches instead. Bundling from anything but a + /// host filesystem needs the seam to describe links first. fn dir(&mut self, src_path: &Path, archive_prefix: &str) -> Result<(), BundleError> { self.builder .append_dir_all(archive_prefix, src_path.as_std_path()) From 93758eedd963e642ea0ddb9162a8e61e274e45e8 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 09:14:46 -0700 Subject: [PATCH 12/12] fix: windows divergence in path handling --- crates/icp-project/src/files.rs | 49 +++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 9f3618c6b..47fa6cc91 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -230,10 +230,12 @@ pub enum GlobError { /// `**` standing for any number of directories — the same shape `glob` /// supports, and the same one the manifest reference documents. /// -/// A component that cannot match anything is instead resolved the way joining -/// it onto `base` would resolve it: `..` climbs, and an absolute pattern starts -/// from its own root with `base` dropped. So a pattern with no metacharacters -/// at all names the same path here as `base.join(pattern)` does. +/// A component with nothing to match against is instead resolved the way +/// joining it onto `base` would resolve it: `..` climbs, and an absolute +/// pattern starts from its own root with `base` dropped. A component with no +/// metacharacter is joined too, since it names one path rather than describing +/// a set of them, so a pattern with no metacharacters at all names the same +/// path here as `base.join(pattern)` does. /// /// Only paths that exist are returned, and each directory's entries are listed /// in sorted order, so the result is the same on every run. `**` descends into @@ -261,14 +263,16 @@ pub async fn expand_glob( Utf8Component::CurDir => {} // No listing ever turns up an entry named `..`, so this names a - // directory rather than matching one. It still has to be one, or - // the pattern describes no path from here. + // directory rather than matching one: the directory above `dir`, + // which is there only where `dir` itself is. Asking about `dir` + // rather than about the joined path is what settles that — Windows + // resolves a trailing `..` by spelling rather than by lookup, and + // answers for `nowhere/..` as readily as for a path that is there. Utf8Component::ParentDir => { let mut next = Vec::new(); for dir in &frontier { - let parent = dir.join(".."); - if files.is_dir(&parent).await { - next.push(parent); + if files.is_dir(dir).await { + next.push(dir.join("..")); } } frontier = next; @@ -319,6 +323,24 @@ pub async fn expand_glob( frontier = reached; } + // A component with no metacharacter matches the single name it + // spells, so it too is joined rather than looked for in a listing. + // Windows spells one file several ways — a case the directory + // entry does not use, or a short name like `RUNNER~1` — and each + // of them opens the file, so looking for the spelling among the + // entries would turn a pattern that names a path into one that + // matches nothing. + Utf8Component::Normal(component) if !component.contains(['*', '?', '[']) => { + let mut next = Vec::new(); + for dir in &frontier { + let entry = dir.join(component); + if files.exists(&entry).await { + next.push(entry); + } + } + frontier = next; + } + Utf8Component::Normal(component) => { let matcher = glob::Pattern::new(component).context(PatternSnafu { pattern: pattern.to_owned(), @@ -420,6 +442,15 @@ mod tests { ); } + /// Windows spells one directory several ways, and a spelling the listing + /// does not use still names it. + #[cfg(windows)] + #[tokio::test] + async fn a_literal_component_need_not_be_spelled_as_the_listing_spells_it() { + let d = tree(&["canisters/a/canister.yaml"]); + assert_eq!(expand(d.path(), "CANISTERS/a").await, ["CANISTERS/a"]); + } + /// `**` stands for zero or more directories, so a pattern that uses it also /// matches at the depth where it stands for none. #[tokio::test]