From 3f09d8d4625be4fdce3a2db1fa769ca76ca9fd03 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Tue, 20 Jun 2023 16:17:41 +0800 Subject: [PATCH] Add proper error handling to the Source trait --- lib/cli/src/commands/run.rs | 12 +-- lib/wasix/src/bin_factory/binary_package.rs | 16 +++- .../src/runtime/resolver/filesystem_source.rs | 12 +-- .../src/runtime/resolver/in_memory_source.rs | 14 +++- lib/wasix/src/runtime/resolver/inputs.rs | 10 ++- lib/wasix/src/runtime/resolver/mod.rs | 6 +- .../src/runtime/resolver/multi_source.rs | 73 +++++++++++++++--- lib/wasix/src/runtime/resolver/resolve.rs | 39 ++++++++-- lib/wasix/src/runtime/resolver/source.rs | 74 ++++++++++++++++--- lib/wasix/src/runtime/resolver/wapm_source.rs | 45 ++++++----- lib/wasix/src/runtime/resolver/web_source.rs | 17 +++-- tests/integration/cli/tests/run.rs | 8 +- 12 files changed, 241 insertions(+), 85 deletions(-) diff --git a/lib/cli/src/commands/run.rs b/lib/cli/src/commands/run.rs index efef3e8343b..82d0b17ff65 100644 --- a/lib/cli/src/commands/run.rs +++ b/lib/cli/src/commands/run.rs @@ -33,7 +33,10 @@ use wasmer_registry::Package; use wasmer_wasix::{ bin_factory::BinaryPackage, runners::{MappedDirectory, Runner}, - runtime::{package_loader::PackageLoader, resolver::PackageSpecifier}, + runtime::{ + package_loader::PackageLoader, + resolver::{PackageSpecifier, QueryError}, + }, WasiError, }; use wasmer_wasix::{ @@ -117,10 +120,7 @@ impl Run { // something that displays progress let monitoring_runtime = MonitoringRuntime::new(runtime, pb.clone()); - let target = self - .input - .resolve_target(&monitoring_runtime, &pb) - .with_context(|| format!("Unable to resolve \"{}\"", self.input))?; + let target = self.input.resolve_target(&monitoring_runtime, &pb)?; pb.finish_and_clear(); @@ -841,7 +841,7 @@ impl wasmer_wasix::runtime::resolver::Source for MonitoringSource { async fn query( &self, package: &PackageSpecifier, - ) -> Result, Error> { + ) -> Result, QueryError> { self.progress.set_message(format!("Looking up {package}")); self.inner.query(package).await } diff --git a/lib/wasix/src/bin_factory/binary_package.rs b/lib/wasix/src/bin_factory/binary_package.rs index f2f10508277..31bddc9361e 100644 --- a/lib/wasix/src/bin_factory/binary_package.rs +++ b/lib/wasix/src/bin_factory/binary_package.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use anyhow::Context; use derivative::*; use once_cell::sync::OnceCell; use semver::Version; @@ -9,7 +10,7 @@ use webc::{compat::SharedBytes, Container}; use crate::{ runtime::{ module_cache::ModuleHash, - resolver::{PackageId, PackageInfo, PackageSpecifier}, + resolver::{PackageId, PackageInfo, PackageSpecifier, ResolveError}, }, Runtime, }; @@ -100,11 +101,20 @@ impl BinaryPackage { runtime: &dyn Runtime, ) -> Result { let source = runtime.source(); - let root_summary = source.latest(specifier).await?; + let root_summary = + source + .latest(specifier) + .await + .map_err(|error| ResolveError::Registry { + package: specifier.clone(), + error, + })?; let root = runtime.package_loader().load(&root_summary).await?; let id = root_summary.package_id(); - let resolution = crate::runtime::resolver::resolve(&id, &root_summary.pkg, &source).await?; + let resolution = crate::runtime::resolver::resolve(&id, &root_summary.pkg, &source) + .await + .context("Dependency resolution failed")?; let pkg = runtime .package_loader() .load_package_tree(&root, &resolution) diff --git a/lib/wasix/src/runtime/resolver/filesystem_source.rs b/lib/wasix/src/runtime/resolver/filesystem_source.rs index 88bacfa5661..9be00fac897 100644 --- a/lib/wasix/src/runtime/resolver/filesystem_source.rs +++ b/lib/wasix/src/runtime/resolver/filesystem_source.rs @@ -1,8 +1,8 @@ -use anyhow::{Context, Error}; +use anyhow::Context; use webc::compat::Container; use crate::runtime::resolver::{ - DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, Source, WebcHash, + DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, QueryError, Source, WebcHash, }; /// A [`Source`] that knows how to query files on the filesystem. @@ -12,7 +12,7 @@ pub struct FileSystemSource {} #[async_trait::async_trait] impl Source for FileSystemSource { #[tracing::instrument(level = "debug", skip_all, fields(%package))] - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { let path = match package { PackageSpecifier::Path(path) => path.canonicalize().with_context(|| { format!( @@ -20,7 +20,7 @@ impl Source for FileSystemSource { path.display() ) })?, - _ => return Ok(Vec::new()), + _ => return Err(QueryError::Unsupported), }; // FIXME: These two operations will block @@ -32,8 +32,10 @@ impl Source for FileSystemSource { let url = crate::runtime::resolver::utils::url_from_file_path(&path) .ok_or_else(|| anyhow::anyhow!("Unable to turn \"{}\" into a URL", path.display()))?; + let pkg = PackageInfo::from_manifest(container.manifest()) + .context("Unable to determine the package's metadata")?; let summary = PackageSummary { - pkg: PackageInfo::from_manifest(container.manifest())?, + pkg, dist: DistributionInfo { webc: url, webc_sha256, diff --git a/lib/wasix/src/runtime/resolver/in_memory_source.rs b/lib/wasix/src/runtime/resolver/in_memory_source.rs index 35456220aa5..0de44ace5c7 100644 --- a/lib/wasix/src/runtime/resolver/in_memory_source.rs +++ b/lib/wasix/src/runtime/resolver/in_memory_source.rs @@ -7,7 +7,7 @@ use std::{ use anyhow::{Context, Error}; use semver::Version; -use crate::runtime::resolver::{PackageSpecifier, PackageSummary, Source}; +use crate::runtime::resolver::{PackageSpecifier, PackageSummary, QueryError, Source}; /// A [`Source`] that tracks packages in memory. /// @@ -88,7 +88,7 @@ impl InMemorySource { #[async_trait::async_trait] impl Source for InMemorySource { #[tracing::instrument(level = "debug", skip_all, fields(%package))] - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { match package { PackageSpecifier::Registry { full_name, version } => { match self.packages.get(full_name) { @@ -106,12 +106,18 @@ impl Source for InMemorySource { .collect::>(), ); + if matches.is_empty() { + return Err(QueryError::NoMatches { + archived_versions: Vec::new(), + }); + } + Ok(matches) } - None => Ok(Vec::new()), + None => Err(QueryError::NotFound), } } - PackageSpecifier::Url(_) | PackageSpecifier::Path(_) => Ok(Vec::new()), + PackageSpecifier::Url(_) | PackageSpecifier::Path(_) => Err(QueryError::Unsupported), } } } diff --git a/lib/wasix/src/runtime/resolver/inputs.rs b/lib/wasix/src/runtime/resolver/inputs.rs index 64bca33e6c5..92735769e05 100644 --- a/lib/wasix/src/runtime/resolver/inputs.rs +++ b/lib/wasix/src/runtime/resolver/inputs.rs @@ -101,7 +101,15 @@ impl FromStr for PackageSpecifier { impl Display for PackageSpecifier { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - PackageSpecifier::Registry { full_name, version } => write!(f, "{full_name}@{version}"), + PackageSpecifier::Registry { full_name, version } => { + write!(f, "{full_name}")?; + + if !version.comparators.is_empty() { + write!(f, "@{version}")?; + } + + Ok(()) + } PackageSpecifier::Url(url) => Display::fmt(url, f), PackageSpecifier::Path(path) => write!(f, "{}", path.display()), } diff --git a/lib/wasix/src/runtime/resolver/mod.rs b/lib/wasix/src/runtime/resolver/mod.rs index d5497711eba..c520a2c5527 100644 --- a/lib/wasix/src/runtime/resolver/mod.rs +++ b/lib/wasix/src/runtime/resolver/mod.rs @@ -16,13 +16,13 @@ pub use self::{ Command, Dependency, DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, WebcHash, }, - multi_source::MultiSource, + multi_source::{MultiSource, MultiSourceStrategy}, outputs::{ DependencyGraph, Edge, ItemLocation, Node, PackageId, Resolution, ResolvedFileSystemMapping, ResolvedPackage, }, - resolve::resolve, - source::Source, + resolve::{resolve, ResolveError}, + source::{QueryError, Source}, wapm_source::WapmSource, web_source::WebSource, }; diff --git a/lib/wasix/src/runtime/resolver/multi_source.rs b/lib/wasix/src/runtime/resolver/multi_source.rs index 5d71315a9c1..e7782418917 100644 --- a/lib/wasix/src/runtime/resolver/multi_source.rs +++ b/lib/wasix/src/runtime/resolver/multi_source.rs @@ -1,11 +1,16 @@ use std::sync::Arc; -use anyhow::Error; - -use crate::runtime::resolver::{PackageSpecifier, PackageSummary, Source}; +use crate::runtime::resolver::{PackageSpecifier, PackageSummary, QueryError, Source}; /// A [`Source`] that works by querying multiple [`Source`]s in succession. /// +/// # Error Handling +/// +/// A [`Source`] implementation can return certain non-fatal errors and, +/// depending on the [`MultiSourceStrategy`], the [`MultiSource`] can choose to +/// deal with it in different ways. Sometimes +/// +/// /// The first [`Source`] to return one or more [`Summaries`][PackageSummary] /// will be treated as the canonical source for that [`Dependency`][dep] and no /// further [`Source`]s will be queried. @@ -14,37 +19,85 @@ use crate::runtime::resolver::{PackageSpecifier, PackageSummary, Source}; #[derive(Debug, Clone)] pub struct MultiSource { sources: Vec>, + strategy: MultiSourceStrategy, } impl MultiSource { pub const fn new() -> Self { MultiSource { sources: Vec::new(), + strategy: MultiSourceStrategy::default(), } } pub fn add_source(&mut self, source: impl Source + Send + Sync + 'static) -> &mut Self { - self.add_shared_source(Arc::new(source)); - self + self.add_shared_source(Arc::new(source)) } pub fn add_shared_source(&mut self, source: Arc) -> &mut Self { self.sources.push(source); self } + + /// Override the strategy used when a [`Source`] returns a non-fatal error. + pub fn with_strategy(self, strategy: MultiSourceStrategy) -> Self { + MultiSource { strategy, ..self } + } } #[async_trait::async_trait] impl Source for MultiSource { #[tracing::instrument(level = "debug", skip_all, fields(%package))] - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { for source in &self.sources { - let result = source.query(package).await?; - if !result.is_empty() { - return Ok(result); + match source.query(package).await { + Ok(summaries) => return Ok(summaries), + Err(QueryError::Unsupported) if self.strategy.continue_if_unsupported => continue, + Err(QueryError::NotFound) if self.strategy.continue_if_not_found => continue, + Err(QueryError::NoMatches { .. }) if self.strategy.continue_if_no_matches => { + continue + } + Err(e) => return Err(e), } } - anyhow::bail!("Unable to find any packages that satisfy the query") + Err(QueryError::NotFound) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct MultiSourceStrategy { + /// If encountered, treat [`QueryError::Unsupported`] as a non-fatal error + /// and query the next [`Source`] in turn. + /// + /// This flag is **enabled** by default. + pub continue_if_unsupported: bool, + /// If encountered, treat [`QueryError::NotFound`] as a non-fatal error and + /// query the next [`Source`] in turn. + /// + /// This flag is **enabled** by default and can be used to let earlier + /// [`Source`]s "override" later ones. + pub continue_if_not_found: bool, + /// If encountered, treat [`QueryError::NoMatches`] as a non-fatal error and + /// query the next [`Source`] in turn. + /// + /// This flag is **disabled** by default. + pub continue_if_no_matches: bool, +} + +impl MultiSourceStrategy { + pub const fn default() -> Self { + MultiSourceStrategy { + continue_if_unsupported: true, + continue_if_not_found: true, + continue_if_no_matches: true, + } + } +} + +impl Default for MultiSourceStrategy { + fn default() -> Self { + MultiSourceStrategy::default() } } diff --git a/lib/wasix/src/runtime/resolver/resolve.rs b/lib/wasix/src/runtime/resolver/resolve.rs index c3e9776da16..14c45a7bcaf 100644 --- a/lib/wasix/src/runtime/resolver/resolve.rs +++ b/lib/wasix/src/runtime/resolver/resolve.rs @@ -11,8 +11,8 @@ use semver::Version; use crate::runtime::resolver::{ outputs::{Edge, Node}, - DependencyGraph, ItemLocation, PackageId, PackageInfo, PackageSummary, Resolution, - ResolvedPackage, Source, + DependencyGraph, ItemLocation, PackageId, PackageInfo, PackageSpecifier, PackageSummary, + QueryError, Resolution, ResolvedPackage, Source, }; use super::ResolvedFileSystemMapping; @@ -33,8 +33,12 @@ pub async fn resolve( #[derive(Debug, thiserror::Error)] pub enum ResolveError { - #[error(transparent)] - Registry(anyhow::Error), + #[error("{}", registry_error_message(.package))] + Registry { + package: PackageSpecifier, + #[source] + error: QueryError, + }, #[error("Dependency cycle detected: {}", print_cycle(_0))] Cycle(Vec), #[error( @@ -47,6 +51,21 @@ pub enum ResolveError { }, } +fn registry_error_message(specifier: &PackageSpecifier) -> String { + match specifier { + PackageSpecifier::Registry { full_name, version } if version.comparators.is_empty() => { + format!("Unable to find \"{full_name}\" in the registry") + } + PackageSpecifier::Registry { full_name, version } => { + format!("Unable to find \"{full_name}@{version}\" in the registry") + } + PackageSpecifier::Url(url) => format!("Unable to resolve \"{url}\""), + PackageSpecifier::Path(path) => { + format!("Unable to load \"{}\" from disk", path.display()) + } + } +} + impl ResolveError { pub fn as_cycle(&self) -> Option<&[PackageId]> { match self { @@ -117,10 +136,14 @@ async fn discover_dependencies( // doing this more rigorously, we would be narrowing the version // down using existing requirements and trying to reuse the same // dependency when possible. - let dep_summary = source - .latest(&dep.pkg) - .await - .map_err(ResolveError::Registry)?; + let dep_summary = + source + .latest(&dep.pkg) + .await + .map_err(|error| ResolveError::Registry { + package: dep.pkg.clone(), + error, + })?; let dep_id = dep_summary.package_id(); let PackageSummary { pkg, dist } = dep_summary; diff --git a/lib/wasix/src/runtime/resolver/source.rs b/lib/wasix/src/runtime/resolver/source.rs index bcaf8b1c03f..de4a2584764 100644 --- a/lib/wasix/src/runtime/resolver/source.rs +++ b/lib/wasix/src/runtime/resolver/source.rs @@ -1,6 +1,4 @@ -use std::fmt::Debug; - -use anyhow::Error; +use std::fmt::{Debug, Display}; use crate::runtime::resolver::{PackageSpecifier, PackageSummary}; @@ -12,22 +10,23 @@ pub trait Source: Sync + Debug { /// /// # Assumptions /// - /// It is not an error if there are no package versions that may satisfy - /// the dependency, even if the [`Source`] doesn't know of a package - /// with that name. + /// If this method returns a successful result, it is guaranteed that there + /// will be at least one [`PackageSummary`], otherwise implementations + /// should return [`QueryError::NotFound`] or [`QueryError::NoMatches`]. /// /// [dep]: crate::runtime::resolver::Dependency - /// [reg]: crate::runtime::resolver::Registry - async fn query(&self, package: &PackageSpecifier) -> Result, Error>; + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError>; /// Run [`Source::query()`] and get the [`PackageSummary`] for the latest /// version. - async fn latest(&self, pkg: &PackageSpecifier) -> Result { + async fn latest(&self, pkg: &PackageSpecifier) -> Result { let candidates = self.query(pkg).await?; candidates .into_iter() .max_by(|left, right| left.pkg.version.cmp(&right.pkg.version)) - .ok_or_else(|| Error::msg("Couldn't find a package version satisfying that constraint")) + .ok_or(QueryError::NoMatches { + archived_versions: Vec::new(), + }) } } @@ -37,7 +36,60 @@ where D: std::ops::Deref + Debug + Send + Sync, S: Source + ?Sized + Send + Sync + 'static, { - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { (**self).query(package).await } } + +#[derive(Debug)] +pub enum QueryError { + Unsupported, + NotFound, + NoMatches { + archived_versions: Vec, + }, + Other(anyhow::Error), +} + +impl From for QueryError { + fn from(value: anyhow::Error) -> Self { + QueryError::Other(value) + } +} + +impl Display for QueryError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + QueryError::Unsupported => { + f.write_str("This type of package specifier isn't supported") + } + QueryError::NotFound => f.write_str("Not found"), + QueryError::NoMatches { archived_versions } => match archived_versions.as_slice() { + [] => f.write_str( + "The package was found, but no published versions matched the constraint", + ), + [version] => write!( + f, + "The only version satisfying the constraint, {version}, is archived" + ), + [first, rest @ ..] => { + let num_others = rest.len(); + write!( + f, + "Unable to satisfy the request. Version {first}, and {num_others} are all archived" + ) + } + }, + QueryError::Other(e) => Display::fmt(e, f), + } + } +} + +impl std::error::Error for QueryError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + QueryError::Other(e) => Some(&**e), + QueryError::Unsupported | QueryError::NotFound | QueryError::NoMatches { .. } => None, + } + } +} diff --git a/lib/wasix/src/runtime/resolver/wapm_source.rs b/lib/wasix/src/runtime/resolver/wapm_source.rs index 7a4cffe4d90..bbebc5ca70d 100644 --- a/lib/wasix/src/runtime/resolver/wapm_source.rs +++ b/lib/wasix/src/runtime/resolver/wapm_source.rs @@ -13,7 +13,8 @@ use webc::metadata::Manifest; use crate::{ http::{HttpClient, HttpRequest, USER_AGENT}, runtime::resolver::{ - DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, Source, WebcHash, + DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, QueryError, Source, + WebcHash, }, }; @@ -147,10 +148,10 @@ impl WapmSource { #[async_trait::async_trait] impl Source for WapmSource { #[tracing::instrument(level = "debug", skip_all, fields(%package))] - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { let (full_name, version_constraint) = match package { PackageSpecifier::Registry { full_name, version } => (full_name, version), - _ => return Ok(Vec::new()), + _ => return Err(QueryError::Unsupported), }; let response: WapmWebQuery = self.lookup_package(full_name).await?; @@ -159,13 +160,24 @@ impl Source for WapmSource { let versions = match response.data.get_package { Some(WapmWebQueryGetPackage { versions }) => versions, - None => return Ok(Vec::new()), + None => return Err(QueryError::NotFound), }; let mut archived_versions = Vec::new(); for pkg_version in versions { - tracing::trace!(?pkg_version, "checking package version"); - let version = Version::parse(&pkg_version.version)?; + tracing::trace!(?pkg_version, "Checking a package version"); + + let version = match Version::parse(&pkg_version.version) { + Ok(v) => v, + Err(e) => { + tracing::debug!( + pkg.version = pkg_version.version.as_str(), + error = &e as &dyn std::error::Error, + "Skipping a version because it doesn't have a valid version numer", + ); + continue; + } + }; if pkg_version.is_archived { tracing::debug!( @@ -191,25 +203,10 @@ impl Source for WapmSource { } if summaries.is_empty() { - match archived_versions.as_slice() { - [] => { - // looks like this package couldn't be satisfied at all. - } - [version] => { - anyhow::bail!( - "The only version satisfying the constraint, {version}, is archived" - ); - } - [first, rest @ ..] => { - let num_others = rest.len(); - anyhow::bail!( - "Unable to satisfy the request, {first}, and {num_others} are all archived" - ); - } - } + Err(QueryError::NoMatches { archived_versions }) + } else { + Ok(summaries) } - - Ok(summaries) } } diff --git a/lib/wasix/src/runtime/resolver/web_source.rs b/lib/wasix/src/runtime/resolver/web_source.rs index 787ca19853d..faece33b55c 100644 --- a/lib/wasix/src/runtime/resolver/web_source.rs +++ b/lib/wasix/src/runtime/resolver/web_source.rs @@ -16,7 +16,8 @@ use webc::compat::Container; use crate::{ http::{HttpClient, HttpRequest}, runtime::resolver::{ - DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, Source, WebcHash, + DistributionInfo, PackageInfo, PackageSpecifier, PackageSummary, QueryError, Source, + WebcHash, }, }; @@ -234,10 +235,10 @@ impl WebSource { #[async_trait::async_trait] impl Source for WebSource { #[tracing::instrument(level = "debug", skip_all, fields(%package))] - async fn query(&self, package: &PackageSpecifier) -> Result, Error> { + async fn query(&self, package: &PackageSpecifier) -> Result, QueryError> { let url = match package { PackageSpecifier::Url(url) => url, - _ => return Ok(Vec::new()), + _ => return Err(QueryError::Unsupported), }; let local_path = self @@ -246,12 +247,16 @@ impl Source for WebSource { .context("Unable to get the locally cached file")?; // FIXME: this will block - let webc_sha256 = WebcHash::for_file(&local_path)?; + let webc_sha256 = WebcHash::for_file(&local_path) + .with_context(|| format!("Unable to hash \"{}\"", local_path.display()))?; // Note: We want to use Container::from_disk() rather than the bytes // our HTTP client gave us because then we can use memory-mapped files - let container = Container::from_disk(&local_path)?; - let pkg = PackageInfo::from_manifest(container.manifest())?; + let container = Container::from_disk(&local_path) + .with_context(|| format!("Unable to load \"{}\"", local_path.display()))?; + let pkg = PackageInfo::from_manifest(container.manifest()) + .context("Unable to determine the package's metadata")?; + let dist = DistributionInfo { webc: url.clone(), webc_sha256, diff --git a/tests/integration/cli/tests/run.rs b/tests/integration/cli/tests/run.rs index dbf88df15f7..00c106a9ac8 100644 --- a/tests/integration/cli/tests/run.rs +++ b/tests/integration/cli/tests/run.rs @@ -425,15 +425,15 @@ fn run_no_imports_wasm_works() { fn run_wasi_works_non_existent() -> anyhow::Result<()> { let assert = Command::new(get_wasmer_path()) .arg("run") - .arg("does/not/exist") + .arg("does-not/exist") .assert() .failure(); assert - .stderr(contains("error: Unable to resolve \"does/not/exist@*\"")) .stderr(contains( - "╰─▶ 1: Unable to find any packages that satisfy the query", - )); + "Unable to find \"does-not/exist\" in the registry", + )) + .stderr(contains("1: Not found")); Ok(()) }