diff --git a/crates/wasm-pkg-common/src/package.rs b/crates/wasm-pkg-common/src/package.rs index 5df67d2..c8a99fa 100644 --- a/crates/wasm-pkg-common/src/package.rs +++ b/crates/wasm-pkg-common/src/package.rs @@ -79,12 +79,25 @@ impl std::fmt::Display for PackageSpec { } /// A package spec combines a [`PackageRef`] with an optional version. -#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)] pub struct PackageSpec { pub package: PackageRef, pub version: Option, } +impl PackageSpec { + /// The requirement used to fetch this package from a registry: exactly its version, or any + /// version (`*`) for a package that names none. + pub fn version_req(&self) -> VersionReq { + match &self.version { + Some(version) => format!("={version}") + .parse() + .expect("an exact version is always a valid requirement"), + None => VersionReq::STAR, + } + } +} + impl PartialEq for PackageSpec { fn eq(&self, other: &str) -> bool { // clippy --fix will create a recursive callsite here if `self.to_string()` is used instead diff --git a/crates/wasm-pkg-core/src/manifest.rs b/crates/wasm-pkg-core/src/manifest.rs index 78dd347..a363aea 100644 --- a/crates/wasm-pkg-core/src/manifest.rs +++ b/crates/wasm-pkg-core/src/manifest.rs @@ -1,13 +1,14 @@ //! Type definitions and functions for working with `wkg.toml` files. use std::{ - collections::HashMap, + collections::{BTreeSet, HashMap}, path::{Path, PathBuf}, }; use anyhow::{Context, Result}; use semver::VersionReq; use serde::{Deserialize, Serialize}; +use wasm_pkg_common::package::{PackageRef, PackageSpec}; mod paths; pub mod workspace; @@ -42,7 +43,8 @@ pub struct Manifest { } impl Manifest { - fn from_toml(contents: &str) -> Result { + /// Parses and validates a manifest from its TOML contents. + pub fn from_toml(contents: &str) -> Result { let manifest: Manifest = toml::from_str(contents)?; manifest.validate()?; Ok(manifest) @@ -79,10 +81,61 @@ impl Manifest { // `Manifest` validations, mirrors cargo's `Workspace::validate` fn validate(&self) -> Result<()> { self.validate_workspace_exclusivity()?; + self.validate_override_keys()?; // Add new validation rules with `self.validate_*()?;` Ok(()) } + /// Checks that override keys parse and that no package is covered by both a bare and a + /// versioned key. + pub(crate) fn validate_override_keys(&self) -> Result<()> { + let Some(overrides) = self.overrides.as_ref() else { + return Ok(()); + }; + // `overrides` is a map, so walk it in a stable order + let sorted_keys: BTreeSet<&str> = overrides.keys().map(String::as_str).collect(); + + // A bare key (`ns:pkg`) covers every version of its package, a versioned key + // (`ns:pkg@1.2.3`) exactly one, so a package may be named by one kind or the other but + // not both + let mut bare: BTreeSet = BTreeSet::new(); + let mut versioned: Vec<(&str, PackageRef)> = Vec::new(); + for key in sorted_keys { + let spec: PackageSpec = key + .parse() + .with_context(|| format!("invalid override key `{key}`"))?; + match spec.version { + Some(_) => versioned.push((key, spec.package)), + None => { + bare.insert(spec.package); + } + } + } + let conflicts: Vec = bare + .iter() + .filter_map(|package| { + let keys: Vec<&str> = versioned + .iter() + .filter(|(_, p)| p == package) + .map(|(key, _)| *key) + .collect(); + (!keys.is_empty()).then(|| { + format!( + "`{package}` covers every version and overlaps `{}`", + keys.join("`, `") + ) + }) + }) + .collect(); + if conflicts.is_empty() { + return Ok(()); + } + anyhow::bail!( + "overrides must not overlap: {} - remove one or the other", + conflicts.join("; ") + ); + } + // no overrides or top-level metadata when workspace is present fn validate_workspace_exclusivity(&self) -> Result<()> { if self.workspace.is_none() { @@ -241,4 +294,68 @@ mod tests { "manifest loaded from file does not match original manifest" ); } + + #[test] + fn override_keys_may_carry_a_version() { + let manifest = Manifest::from_toml( + r#" +[overrides] +"foo:bar@0.1.0" = { path = "bar-0.1.0" } +"foo:bar@0.2.0" = { path = "bar-0.2.0" } +"foo:baz" = { path = "baz" } +"#, + ) + .expect("versioned override keys should be accepted"); + assert_eq!(manifest.overrides.unwrap().len(), 3); + } + + #[test] + fn override_keys_conflict_when_bare_and_versioned() { + let err = Manifest::from_toml( + r#" +[overrides] +"foo:bar" = { path = "bar" } +"foo:bar@0.1.0" = { path = "bar-0.1.0" } +"#, + ) + .expect_err("a bare key alongside a versioned one is ambiguous"); + let err = format!("{err:#}"); + assert!(err.contains("foo:bar@0.1.0"), "unexpected error: {err}"); + } + + #[test] + fn override_key_conflicts_are_all_reported_in_a_stable_order() { + // Two conflicting packages: both must appear, and always in the same order, rather than + // whichever the underlying map happened to yield first. + let err = Manifest::from_toml( + r#" +[overrides] +"zzz:two" = { path = "z" } +"zzz:two@0.2.0" = { path = "z2" } +"aaa:one" = { path = "a" } +"aaa:one@0.1.0" = { path = "a1" } +"#, + ) + .expect_err("both packages conflict"); + let err = format!("{err:#}"); + let aaa = err.find("aaa:one").expect("aaa:one should be reported"); + let zzz = err.find("zzz:two").expect("zzz:two should be reported"); + assert!(aaa < zzz, "conflicts should be sorted: {err}"); + } + + #[test] + fn override_keys_must_parse() { + let err = Manifest::from_toml( + r#" +[overrides] +"not a package ref" = { path = "bar" } +"#, + ) + .expect_err("an unparseable override key should be rejected"); + let err = format!("{err:#}"); + assert!( + err.contains("invalid override key"), + "unexpected error: {err}" + ); + } } diff --git a/crates/wasm-pkg-core/src/resolver.rs b/crates/wasm-pkg-core/src/resolver.rs index 08bba5c..4e3d01c 100644 --- a/crates/wasm-pkg-core/src/resolver.rs +++ b/crates/wasm-pkg-core/src/resolver.rs @@ -2,7 +2,7 @@ // NOTE(thomastaylor312): This is copied and adapted from the `cargo-component` crate: https://github.com/bytecodealliance/cargo-component/blob/f0be1c7d9917aa97e9102e69e3b838dae38d624b/crates/core/src/registry.rs use std::{ - collections::{BTreeSet, HashMap, HashSet, hash_map}, + collections::{BTreeMap, BTreeSet, HashMap, HashSet, hash_map}, fmt::Debug, ops::{Deref, DerefMut}, path::{Path, PathBuf}, @@ -164,6 +164,9 @@ impl RegistryResolution { pub struct LocalResolution { /// The name of the dependency that was resolved. pub name: PackageRef, + /// The version declared by the package at `path`. `None` means that package + /// declares no version, which in WIT is a package distinct from any versioned one. + pub version: Option, /// The path to the resolved dependency. pub path: PathBuf, } @@ -189,11 +192,12 @@ impl DependencyResolution { /// Gets the resolved version. /// - /// Returns `None` if the dependency is not resolved from a registry package. + /// `None` means the resolved package declares no version, which is only possible for a local + /// resolution: a registry release always has one. pub fn version(&self) -> Option<&Version> { match self { Self::Registry(res) => Some(&res.version), - Self::Local(_) => None, + Self::Local(res) => res.version.as_ref(), } } @@ -348,8 +352,14 @@ pub struct DependencyResolver<'a> { client: CachingClient, lock_file: Option<&'a LockFile>, packages: HashMap>, - dependencies: HashMap, - resolutions: DependencyResolutionMap, + dependencies: HashMap, + /// Overrides pointing at a path on disk, held as paths until [`Self::resolve`] reads each + /// one, so a [`LocalResolution`] never exists without the version found at its path. + local_overrides: BTreeMap, + /// Packages with a bare override (`foo:bar`). This is the one place a key means a *range*: + /// a bare override covers every version of its package. Keys everywhere else, including + /// those with no version, name exactly one package, as they do in WIT. + bare_overrides: HashSet, } impl<'a> DependencyResolver<'a> { @@ -368,9 +378,10 @@ impl<'a> DependencyResolver<'a> { Ok(DependencyResolver { client, lock_file, - resolutions: Default::default(), packages: Default::default(), dependencies: Default::default(), + local_overrides: Default::default(), + bare_overrides: Default::default(), }) } @@ -387,38 +398,74 @@ impl<'a> DependencyResolver<'a> { Ok(DependencyResolver { client, lock_file, - resolutions: Default::default(), packages: Default::default(), dependencies: Default::default(), + local_overrides: Default::default(), + bare_overrides: Default::default(), }) } /// Add a dependency to the resolver. If the dependency already exists, then it will be ignored. /// To override an existing dependency, use [`override_dependency`](Self::override_dependency). + /// + /// A key without a version covers every version of the package. Callers adding several + /// overrides must reject overlapping keys first, as `Manifest::validate_override_keys` does; + /// overlapping keys apply in insertion order. pub async fn add_dependency( &mut self, - name: &PackageRef, + key: &PackageSpec, dependency: &Dependency, ) -> Result<()> { - self.add_dependency_internal(name, dependency, false).await + self.add_dependency_internal(key, dependency, false).await?; + self.record_bare_override(key); + Ok(()) } /// Add a dependency to the resolver. If the dependency already exists, then it will be /// overridden. pub async fn override_dependency( &mut self, - name: &PackageRef, + key: &PackageSpec, dependency: &Dependency, ) -> Result<()> { - self.add_dependency_internal(name, dependency, true).await + self.add_dependency_internal(key, dependency, true).await?; + self.record_bare_override(key); + Ok(()) + } + + /// A bare override covers every version of its package, so versioned WIT imports of it + /// (`foo:bar@1.2.0`) are skipped instead of fetched. The resolver does not police overlap + /// between override keys; callers reject that first (`Manifest::validate_override_keys`), + /// and overlapping keys added anyway apply in insertion order. + fn record_bare_override(&mut self, key: &PackageSpec) { + if key.version.is_none() { + self.bare_overrides.insert(key.package.clone()); + } + } + + /// Returns whether this key is already accounted for: the same key was added before, or a + /// bare override covers every version of its package. + /// + /// A key skipped for the latter reason is never recorded anywhere — it is satisfied by the + /// bare override's own resolution, not by one of its own, so [`DependencyResolutionMap`] has + /// no entry for it. + fn is_covered(&self, key: &PackageSpec) -> bool { + self.local_overrides.contains_key(key) + || self.dependencies.contains_key(key) + || self.bare_overrides.contains(&key.package) } async fn add_dependency_internal( &mut self, - name: &PackageRef, + key: &PackageSpec, dependency: &Dependency, force_override: bool, ) -> Result<()> { + if !force_override && self.is_covered(key) { + tracing::debug!(%key, %dependency, "dependency already exists and override is not set, ignoring"); + return Ok(()); + } + let name = &key.package; match dependency { Dependency::Package(package) => { // Dependency comes from a registry, add a dependency to the resolver @@ -443,16 +490,9 @@ impl<'a> DependencyResolver<'a> { _ => None, }; - // So if it wasn't already fetched first? then we'll try and resolve it later, and the override - // is not present there for some reason - if !force_override - && (self.resolutions.contains_key(name) || self.dependencies.contains_key(name)) - { - tracing::debug!(%name, %dependency, "dependency already exists and override is not set, ignoring"); - return Ok(()); - } + self.local_overrides.remove(key); self.dependencies.insert( - name.to_owned(), + key.to_owned(), RegistryDependency { package: package_name, version: package.version.clone(), @@ -461,59 +501,30 @@ impl<'a> DependencyResolver<'a> { ); } Dependency::Local(p) => { - let res = DependencyResolution::Local(LocalResolution { - name: name.clone(), - path: p.clone(), - }); - - // This is a bit of a hack, but if there are multiple local dependencies that are - // nested and overridden, getting the packages from the local package treats _all_ - // deps as registry deps. So if we're handling a local path and the dependencies - // have a registry package already, override it. Otherwise follow normal overrides. - // We should definitely fix this and change where we resolve these things - let should_insert = force_override - || self.dependencies.contains_key(name) - || !self.resolutions.contains_key(name); - if !should_insert { - tracing::debug!(%name, "dependency already exists and registry override is not set, ignoring"); - return Ok(()); - } - - // Because we got here, we should remove anything from dependencies that is the same - // package because we're overriding with the local package. Technically we could be - // clever and just do this in the boolean above, but I'm paranoid - self.dependencies.remove(name); - - // Now that we check we haven't already inserted this dep, get the packages from the - // local dependency and add those to the resolver before adding the dependency - let (_, packages) = get_packages(p) - .context("Error getting dependent packages from local dependency")?; - Box::pin(self.add_packages(packages)) - .await - .context("Error adding packages to resolver for local dependency")?; - - let prev = self.resolutions.insert(name.clone(), res); - assert!(prev.is_none()); + self.dependencies.remove(key); + self.local_overrides.insert(key.clone(), p.clone()); } } Ok(()) } - /// A helper function for adding an iterator of package refs and their associated version - /// requirements to the resolver + /// A helper function for adding an iterator of packages named by a WIT file to the resolver. pub async fn add_packages( &mut self, - packages: impl IntoIterator, + packages: impl IntoIterator, ) -> Result<()> { - for (package, req) in packages { - self.add_dependency( - &package, + for spec in packages { + // Not `add_dependency`: a versionless WIT import names the unversioned package and + // must not be recorded as a bare override covering every version + self.add_dependency_internal( + &spec, &Dependency::Package(RegistryPackage { - name: Some(package.clone()), - version: req, + name: Some(spec.package.clone()), + version: spec.version_req(), registry: None, }), + false, ) .await?; } @@ -526,8 +537,34 @@ impl<'a> DependencyResolver<'a> { /// /// Returns the dependency resolution map. pub async fn resolve(mut self) -> Result { - let mut resolutions = self.resolutions; - for (name, dependency) in self.dependencies.into_iter() { + let mut resolutions = DependencyResolutionMap::default(); + + // Reading a local package is what turns its override key into a resolution. The registry + // packages it names are added here rather than at override time, so that every override is + // already known and anything one covers is skipped; `self.local_overrides` stays intact for + // `is_covered`, which makes this loop independent of the order the keys come out in. + let local_overrides: Vec<(PackageSpec, PathBuf)> = self + .local_overrides + .iter() + .map(|(key, path)| (key.clone(), path.clone())) + .collect(); + for (key, path) in local_overrides { + let (found, packages) = get_packages(&path) + .context("Error getting dependent packages from local dependency")?; + check_local_override(&key, &found, &path)?; + let resolution = DependencyResolution::Local(LocalResolution { + name: found.package, + version: found.version, + path, + }); + resolutions.insert(key, resolution); + self.add_packages(packages) + .await + .context("Error adding packages to resolver for local dependency")?; + } + + for (key, dependency) in self.dependencies.into_iter() { + let name = &key.package; // We need to clone a handle to the client because we mutably borrow self below. Might // be worth replacing the mutable borrow with a RwLock down the line. let client = self.client.clone(); @@ -604,7 +641,7 @@ impl<'a> DependencyResolver<'a> { registry: self.client.client().ok().and_then(|client| { client .config() - .resolve_registry(&name) + .resolve_registry(name) .map(ToString::to_string) }), requirement: dependency.version.clone(), @@ -612,13 +649,29 @@ impl<'a> DependencyResolver<'a> { digest: release.content_digest.clone(), client: self.client.clone(), }; - resolutions.insert(name, DependencyResolution::Registry(resolution)); + resolutions.insert(key, DependencyResolution::Registry(resolution)); } Ok(resolutions) } } +/// An override key names the package it stands in for, so the package on disk has to be that +/// package: a bare key must match the name, a versioned key the name and the version exactly. +fn check_local_override(key: &PackageSpec, found: &PackageSpec, path: &Path) -> Result<()> { + let matches = match key.version { + Some(_) => key == found, + None => key.package == found.package, + }; + if matches { + return Ok(()); + } + bail!( + "override `{key}` points at {path}, which is package `{found}` - fix the key or the path", + path = path.display() + ) +} + async fn load_package<'b>( packages: &'b mut HashMap>, client: &CachingClient, @@ -660,18 +713,29 @@ fn find_latest_release<'a>( /// Represents a map of dependency resolutions. /// -/// The key to the map is the package name of the dependency. +/// Each key is a dependency's package plus the version it was requested at. This lets a world +/// naming several versions of the same package resolve all of them, not just one. +/// +/// Under a versioned key the resolution carries that exact version. Under a bare key it carries +/// whichever version was found: the package's own at a local path, the registry's pick otherwise. +/// +/// A key is a requirement and its value the resolution that satisfies it, so this map is the +/// requirement-to-resolution function: one requirement never maps to two resolutions. The reverse +/// can happen — a bare override key is one resolution satisfying every versioned requirement for +/// its package — which is why this map can hold fewer entries than there were imports. The +/// requirements a bare override absorbs this way are never recorded here; the resolver's internal +/// `is_covered` check is what skips them before they would be. #[derive(Debug, Clone, Default)] -pub struct DependencyResolutionMap(HashMap); +pub struct DependencyResolutionMap(HashMap); -impl AsRef> for DependencyResolutionMap { - fn as_ref(&self) -> &HashMap { +impl AsRef> for DependencyResolutionMap { + fn as_ref(&self) -> &HashMap { &self.0 } } impl Deref for DependencyResolutionMap { - type Target = HashMap; + type Target = HashMap; fn deref(&self) -> &Self::Target { &self.0 diff --git a/crates/wasm-pkg-core/src/wit.rs b/crates/wasm-pkg-core/src/wit.rs index 5fcf934..49e2cf2 100644 --- a/crates/wasm-pkg-core/src/wit.rs +++ b/crates/wasm-pkg-core/src/wit.rs @@ -9,7 +9,7 @@ use std::{ use anyhow::{Context as _, Result, bail}; use indexmap::IndexMap; use petgraph::{Direction, data::Build}; -use semver::{Version, VersionReq}; +use semver::Version; use wasm_metadata::{AddMetadata, AddMetadataField}; use wasm_pkg_client::{ PackageRef, @@ -146,14 +146,12 @@ pub async fn fetch_dependencies( populate_dependencies(wit_dir, &dependencies, output).await } -/// Generate the list of all packages and their version requirement from the given path (a directory -/// or file). +/// Generate the list of all packages named by the given path (a directory or file), each with the +/// version the WIT asks for, if any. /// /// This is a lower level function exposed for convenience that is used by higher level functions /// for resolving dependencies. -pub fn get_packages( - path: impl AsRef, -) -> Result<(PackageSpec, HashSet<(PackageRef, VersionReq)>)> { +pub fn get_packages(path: impl AsRef) -> Result<(PackageSpec, HashSet)> { let path = path.as_ref(); // Build a package group out of a single file or a directory @@ -201,7 +199,7 @@ pub fn get_packages( }; // Get all package refs from the main package and then from any nested packages - let packages: HashSet<(PackageRef, VersionReq)> = + let packages: HashSet = packages_from_foreign_deps(group.main.foreign_deps.into_keys()) .chain( group @@ -241,7 +239,7 @@ pub(crate) fn get_local_dependencies( } for ((spec, _), deps) in pkg_trees { // TODO handle version matching for dependencies - for (dep, _version) in deps { + for PackageSpec { package: dep, .. } in deps { if let Some(&(dep, _)) = indices.get(&dep) { let pkg = &spec.package; let (id, _) = indices[pkg]; @@ -279,16 +277,26 @@ pub async fn resolve_dependencies( lock_file: Option<&LockFile>, client: CachingClient, ) -> Result { + // A manifest built in code skips TOML-load validation, so check the override keys here too + manifest.validate_override_keys()?; + let mut resolver = DependencyResolver::new_with_client(client, lock_file)?; // add deps from manifest first in case they're local deps and then add deps from the directory if let Some(overrides) = manifest.overrides.as_ref() { tracing::debug!("detected manifest overrides"); for (pkg, ovr) in overrides.iter() { - let pkg: PackageRef = pkg.parse().context("Unable to parse as a package ref")?; + // `"ns:pkg"` overrides every version of the package, `"ns:pkg@1.2.3"` just that one + let key: PackageSpec = pkg + .parse() + .with_context(|| format!("invalid override key `{pkg}`"))?; let dep = match (ovr.path.as_ref(), ovr.version.as_ref()) { (Some(path), v) => { if v.is_some() { - tracing::warn!("Ignoring version override for local package"); + tracing::warn!( + %key, + "the `version` field has no effect on a local (path) override; the \ + key alone decides which version it applies to", + ); } let path = tokio::fs::canonicalize(path).await.with_context(|| { format!("resolving local dependency {}", path.display()) @@ -296,7 +304,7 @@ pub async fn resolve_dependencies( Dependency::Local(path) } (None, Some(version)) => Dependency::Package(RegistryPackage { - name: Some(pkg.clone()), + name: Some(key.package.clone()), version: version.to_owned(), registry: None, }), @@ -308,7 +316,7 @@ pub async fn resolve_dependencies( tracing::debug!(dependency = %dep); resolver - .add_dependency(&pkg, &dep) + .add_dependency(&key, &dep) .await .with_context(|| format!("unable to add dependency {dep}"))?; } @@ -419,19 +427,13 @@ async fn write_wasm_deps( fn packages_from_foreign_deps( deps: impl IntoIterator, -) -> impl Iterator { +) -> impl Iterator { deps.into_iter().filter_map(|dep| { - let name = PackageRef::new(dep.namespace.parse().ok()?, dep.name.parse().ok()?); - let version = match dep.version { - Some(v) => format!("={v}"), - None => "*".to_string(), - }; - Some(( - name, - version - .parse() - .expect("Unable to parse into version request, this is programmer error"), - )) + let package = PackageRef::new(dep.namespace.parse().ok()?, dep.name.parse().ok()?); + Some(PackageSpec { + package, + version: dep.version, + }) }) } @@ -551,3 +553,52 @@ fn name_from_package_name(package_name: &PackageName) -> String { let package_name_str = package_name.to_string(); package_name_str.replace([':', '@'], "-") } + +#[cfg(test)] +mod tests { + use super::*; + + /// An override key and the WIT import it targets must produce the same key, versioned or not. + #[test] + fn override_key_matches_foreign_dep() { + for version in [ + None, + Some("0.1.0"), + Some("1.2.3"), + Some("0.2.0-draft"), + Some("0.2.0-alpha.1"), + ] { + let from_wit = packages_from_foreign_deps([PackageName { + namespace: "foo".to_string(), + name: "bar".to_string(), + version: version.map(|v| v.parse().unwrap()), + }]) + .next() + .expect("foreign dep should yield a package"); + + let key = match version { + Some(version) => format!("foo:bar@{version}"), + None => "foo:bar".to_string(), + }; + let from_key: PackageSpec = key.parse().unwrap(); + + assert_eq!(from_key, from_wit, "key for {version:?}"); + let req = from_key.version_req(); + assert_eq!( + req.to_string(), + version.map(|v| format!("={v}")).unwrap_or("*".to_string()), + "requirement for {version:?}" + ); + } + } + + #[test] + fn malformed_override_keys_are_rejected() { + for key in ["foo:bar@", "foo:bar@not-a-version", "not a package ref"] { + assert!( + key.parse::().is_err(), + "`{key}` should not parse" + ); + } + } +} diff --git a/crates/wasm-pkg-core/tests/fetch.rs b/crates/wasm-pkg-core/tests/fetch.rs index a56685c..dc290f2 100644 --- a/crates/wasm-pkg-core/tests/fetch.rs +++ b/crates/wasm-pkg-core/tests/fetch.rs @@ -92,38 +92,17 @@ async fn test_transitive_local(#[values(OutputType::Wasm, OutputType::Wit)] outp let mut lock = LockFile::new_with_path([], &lock_file) .await .expect("Should be able to create a new lock file"); - // ```toml - // [overrides] - // "example-b:bar" = { "path" = "../example-b/wit" } - // "example-c:baz" = { "path" = "../example-c/wit" } - // "example-c:nested" = { "path" = "../example-c/wit/nested" } - // ``` - let manifest = Manifest { - overrides: Some(HashMap::from([ - ( - "example-b:bar".to_string(), - Override { - path: Some(fixture_path.join("example-b/wit")), - version: None, - }, - ), - ( - "example-c:baz".to_string(), - Override { - path: Some(fixture_path.join("example-c/wit")), - version: None, - }, - ), - ( - "example-c:nested".to_string(), - Override { - path: Some(fixture_path.join("example-c/wit/nested")), - version: None, - }, - ), - ])), - ..Default::default() - }; + // Override paths resolve against the working directory, so they must be absolute here + let manifest = Manifest::from_toml(&format!( + r#" +[overrides] +"example-b:bar" = {{ path = '{root}/example-b/wit' }} +"example-c:baz" = {{ path = '{root}/example-c/wit' }} +"example-c:nested" = {{ path = '{root}/example-c/wit/nested' }} +"#, + root = fixture_path.display() + )) + .expect("manifest should parse"); let (_temp_cache, client) = common::get_client().await.unwrap(); // If overrides didn't properly resolve, this will fail @@ -158,6 +137,230 @@ async fn test_transitive_local(#[values(OutputType::Wasm, OutputType::Wit)] outp ); } +/// A world can name several versions of the same package, so an override key may carry an exact +/// version to scope it to just one of them. +#[rstest] +#[tokio::test] +async fn test_multi_version_local(#[values(OutputType::Wasm, OutputType::Wit)] output: OutputType) { + let (_temp, fixture_path) = common::load_fixture("multi-version-local").await.unwrap(); + let project_path = fixture_path.join("project"); + let lock_file = project_path.join("wkg.lock"); + let mut lock = LockFile::new_with_path([], &lock_file) + .await + .expect("Should be able to create a new lock file"); + let manifest = Manifest::from_toml(&format!( + r#" +[overrides] +"my:local@0.1.0" = {{ path = '{root}/local-dep-0.1.0/wit' }} +"my:local@0.2.0" = {{ path = '{root}/local-dep-0.2.0/wit' }} +"#, + root = fixture_path.display() + )) + .expect("manifest should parse"); + let (_temp_cache, client) = common::get_client().await.unwrap(); + + wit::fetch_dependencies( + &manifest, + project_path.join("wit"), + &mut lock, + client, + output, + ) + .await + .unwrap_or_else(|e| panic!("Should be able to fetch the dependencies: {e:#}")); + + // Both versions must be written out. Before per-version override keys, the two entries + // collided on the package name and only one of them (nondeterministically) survived. + let mut deps_dir = tokio::fs::read_dir(project_path.join("wit/deps")) + .await + .expect("Should be able to read the deps directory"); + let mut deps = Vec::new(); + while let Ok(Some(entry)) = deps_dir.next_entry().await { + deps.push(entry.file_name().to_string_lossy().to_string()); + } + // Local directory deps are written out as directories under both output types + assert_eq!(deps.len(), 2, "expected both versions, got {deps:?}"); + assert!( + deps.contains(&"my-local-0.1.0".to_string()), + "missing my-local-0.1.0 in {deps:?}" + ); + assert!( + deps.contains(&"my-local-0.2.0".to_string()), + "missing my-local-0.2.0 in {deps:?}" + ); + + // each directory must hold its own version's WIT, not two copies of whichever one won + let v1 = read_dep_wit(&project_path.join("wit/deps/my-local-0.1.0")).await; + let v2 = read_dep_wit(&project_path.join("wit/deps/my-local-0.2.0")).await; + assert!(v1.contains("foo: func() -> string"), "0.1.0 body was: {v1}"); + assert!( + v2.contains("foo: func() -> result"), + "0.2.0 body was: {v2}" + ); + + // All dependencies are local, so the lock file should be empty + assert_eq!( + lock.packages.len(), + 0, + "Should have the correct number of packages in the lock file" + ); +} + +/// The same applies to packages coming from a registry rather than from an override: both +/// versions must be fetched and both must be recorded against the one package in the lock file. +#[rstest] +#[tokio::test] +async fn test_multi_version_registry( + #[values(OutputType::Wasm, OutputType::Wit)] output: OutputType, +) { + let (_temp, fixture_path) = common::load_fixture("multi-version-registry") + .await + .unwrap(); + let lock_file = fixture_path.join("wkg.lock"); + let mut lock = LockFile::new_with_path([], &lock_file) + .await + .expect("Should be able to create a new lock file"); + let (_temp_cache, client) = common::get_client().await.unwrap(); + + wit::fetch_dependencies( + &Manifest::default(), + fixture_path.join("wit"), + &mut lock, + client, + output, + ) + .await + .unwrap_or_else(|e| panic!("Should be able to fetch the dependencies: {e:#}")); + + let mut deps_dir = tokio::fs::read_dir(fixture_path.join("wit/deps")) + .await + .expect("Should be able to read the deps directory"); + let mut deps = Vec::new(); + while let Ok(Some(entry)) = deps_dir.next_entry().await { + deps.push(entry.file_name().to_string_lossy().to_string()); + } + let suffix = match output { + OutputType::Wit => "", + OutputType::Wasm => ".wasm", + }; + for version in ["0.2.0", "0.2.1"] { + let expected = format!("wasi-clocks-{version}{suffix}"); + assert!(deps.contains(&expected), "missing {expected} in {deps:?}"); + } + + // Both requirements belong to the one `wasi:clocks` package, so the lock file records a single + // package holding two locked versions. + let clocks = lock + .packages + .iter() + .find(|p| p.name.to_string() == "wasi:clocks") + .expect("wasi:clocks should be in the lock file"); + let mut locked: Vec = clocks + .versions + .iter() + .map(|v| v.version.to_string()) + .collect(); + locked.sort(); + assert_eq!(locked, ["0.2.0", "0.2.1"], "lock file versions: {locked:?}"); +} + +/// A bare key already covers every version, so pairing it with a versioned key for the same +/// package is ambiguous and resolving must reject it rather than pick a winner by map ordering. +#[tokio::test] +async fn test_conflicting_override_keys_rejected() { + let (_temp, fixture_path) = common::load_fixture("multi-version-local").await.unwrap(); + let project_path = fixture_path.join("project"); + let mut lock = LockFile::new_with_path([], project_path.join("wkg.lock")) + .await + .expect("Should be able to create a new lock file"); + // Built directly rather than parsed from TOML, so `Manifest::validate` never ran + let manifest = Manifest { + overrides: Some(HashMap::from([ + ( + "my:local".to_string(), + Override { + path: Some(fixture_path.join("local-dep-0.1.0/wit")), + version: None, + }, + ), + ( + "my:local@0.2.0".to_string(), + Override { + path: Some(fixture_path.join("local-dep-0.2.0/wit")), + version: None, + }, + ), + ])), + ..Default::default() + }; + let (_temp_cache, client) = common::get_client().await.unwrap(); + + let err = wit::fetch_dependencies( + &manifest, + project_path.join("wit"), + &mut lock, + client, + OutputType::Wit, + ) + .await + .expect_err("conflicting override keys should be rejected"); + + let err = format!("{err:#}"); + assert!( + err.contains("my:local@0.2.0") && err.contains("overlap"), + "unexpected error: {err}" + ); +} + +/// A versioned override key must name the package actually at its path; a mismatch is an error +/// naming both the key and what is on disk. +#[tokio::test] +async fn test_override_key_must_match_package_on_disk() { + let (_temp, fixture_path) = common::load_fixture("multi-version-local").await.unwrap(); + let project_path = fixture_path.join("project"); + let mut lock = LockFile::new_with_path([], project_path.join("wkg.lock")) + .await + .expect("Should be able to create a new lock file"); + let manifest = Manifest::from_toml(&format!( + r#" +[overrides] +"my:local@0.1.0" = {{ path = '{root}/local-dep-0.2.0/wit' }} +"#, + root = fixture_path.display() + )) + .expect("manifest should parse"); + let (_temp_cache, client) = common::get_client().await.unwrap(); + + let err = wit::fetch_dependencies( + &manifest, + project_path.join("wit"), + &mut lock, + client, + OutputType::Wit, + ) + .await + .expect_err("an override key that does not match the package on disk should be rejected"); + + let err = format!("{err:#}"); + assert!( + err.contains("override `my:local@0.1.0` points at") + && err.contains("is package `my:local@0.2.0`"), + "unexpected error: {err}" + ); +} + +/// Concatenates every WIT file in a dep directory; the file names inside differ by output type. +async fn read_dep_wit(dir: &Path) -> String { + let mut entries = tokio::fs::read_dir(dir) + .await + .unwrap_or_else(|e| panic!("Should be able to read {}: {e}", dir.display())); + let mut contents = String::new(); + while let Ok(Some(entry)) = entries.next_entry().await { + contents.push_str(&tokio::fs::read_to_string(entry.path()).await.unwrap()); + } + contents +} + async fn build_component(fixture_path: &Path) { let output = Command::new(env!("CARGO")) .current_dir(fixture_path) diff --git a/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.1.0/wit/world.wit b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.1.0/wit/world.wit new file mode 100644 index 0000000..6c0518a --- /dev/null +++ b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.1.0/wit/world.wit @@ -0,0 +1,5 @@ +package my:local@0.1.0; + +interface foo { + foo: func() -> string; +} diff --git a/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.2.0/wit/world.wit b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.2.0/wit/world.wit new file mode 100644 index 0000000..8736692 --- /dev/null +++ b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/local-dep-0.2.0/wit/world.wit @@ -0,0 +1,6 @@ +package my:local@0.2.0; + +interface foo { + // Differs from 0.1.0 so a test can tell which version was written out. + foo: func() -> result; +} diff --git a/crates/wasm-pkg-core/tests/fixtures/multi-version-local/project/wit/world.wit b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/project/wit/world.wit new file mode 100644 index 0000000..a3f1a19 --- /dev/null +++ b/crates/wasm-pkg-core/tests/fixtures/multi-version-local/project/wit/world.wit @@ -0,0 +1,7 @@ +package my:component; + +// Exports two versions of the same package, which is only expressible with per-version overrides. +world component { + export my:local/foo@0.1.0; + export my:local/foo@0.2.0; +} diff --git a/crates/wasm-pkg-core/tests/fixtures/multi-version-registry/wit/world.wit b/crates/wasm-pkg-core/tests/fixtures/multi-version-registry/wit/world.wit new file mode 100644 index 0000000..1ba826a --- /dev/null +++ b/crates/wasm-pkg-core/tests/fixtures/multi-version-registry/wit/world.wit @@ -0,0 +1,7 @@ +package test:multiversion; + +// Imports two versions of the same registry package, which must both be fetched. +world multiversion { + import wasi:clocks/monotonic-clock@0.2.0; + import wasi:clocks/monotonic-clock@0.2.1; +} diff --git a/crates/wkg/src/wit.rs b/crates/wkg/src/wit.rs index 76cb811..2782699 100644 --- a/crates/wkg/src/wit.rs +++ b/crates/wkg/src/wit.rs @@ -238,11 +238,11 @@ impl FetchArgs { .with_context(|| { format!("failed to resolve dependencies for {}", dir.display()) })?; - for (pkg, resolution) in resolved.as_ref() { - if verifier.packages.contains(pkg) { + for (key, resolution) in resolved.as_ref() { + if verifier.packages.contains(&key.package) { continue; } - merged.insert(pkg.clone(), resolution.clone()); + merged.insert(key.clone(), resolution.clone()); } } diff --git a/docs/manifest.md b/docs/manifest.md index bd6b2c9..4ca86af 100644 --- a/docs/manifest.md +++ b/docs/manifest.md @@ -54,6 +54,30 @@ developing two components together. "my:local-dep" = { path = "../local-dep/wit" } ``` +A bare package name applies to every version of that package the WIT names. To +scope an override to one version, suffix the key with that exact version. This +is what lets a world name more than one version of the same package, since each +version can then point somewhere different: + +```toml +[overrides] +"my:local-dep@0.1.0" = { path = "../local-dep-0.1.0/wit" } +"my:local-dep@0.2.0" = { path = "../local-dep-0.2.0/wit" } +``` + +A package cannot have both a bare and a versioned key, since the bare key +already covers every version; such a manifest is rejected rather than resolved +in an unspecified order. + +The key has to describe the package it points at: a bare key must match its +name, and a versioned key its name and version. Pointing `"my:local@0.1.0"` at +a directory whose package is `my:local@0.2.0` is an error rather than a silent +version swap. + +Note that the `version` field is a *registry* version requirement and is +ignored when `path` is set - use a versioned key to select which version an +override applies to. + ### `workspace.members` - Type: list of strings (paths; gitignore-style globs allowed)