feat(core,wkg): resolve multiple versions of the same package - #251
feat(core,wkg): resolve multiple versions of the same package#251Aditya1404Sal wants to merge 3 commits into
Conversation
|
Hi @Aditya1404Sal, thanks for the submission. |
76f0b66 to
70b0b27
Compare
Key dependencies by package plus version requirement rather than by package alone, so a wkg.toml can declare a separate override per version and a world naming two versions of the same package resolves both instead of dropping one
70b0b27 to
6e07006
Compare
Sorry for that, this PR description is indeed ai-assisted. I've rewritten it to my taste. |
@lukewagner apologies for pinging, just wanted to get your feedback and see if this is something we want to affirm for package distribution. |
|
Was chatting a bit with @mkatychev. I'm not sure this is desired behavior. First off, in your example, the major version is 0, so the bump from 0.2, to 0.3 should signify a breaking change. I think at the very least we'd probably want to not resolve multiple versions when the change is breaking. But even if not breaking, I think a wit/interface change is meant to be more intentional than just bumping a library. For example, the bump from wasi 0.2 to 0.3 completely changes dependence on wasi:io. All this said, I do believe that package versions are currently treated as a semver carat, so if a new non breaking version is published, you'll automatically get it, which doesn't feel so far off from the spirit of the PR. I am curious though if you've tried using a file path dependency. It seems like that could potentially meet your needs @Aditya1404Sal without the potential baggage of supporting resolution for multiple package versions. |
|
@macovedj, had some discussion with @salmans @vados-cosmonic about it and it feels like this is something we should allow this; more context here: https://github.com/bytecodealliance/meetings//blob/main/SIG-Packaging/2026/2026-08-19.md#multiple-versions-of-the-same-pacakge-as-dependencies-for-one-package |
| pub struct DependencyKey { | ||
| /// The package the dependency refers to. | ||
| pub package: PackageRef, | ||
| /// The version this dependency was requested at. | ||
| /// | ||
| /// `None` means "every version". This only happens for a bare override key (no `@version`), | ||
| /// which applies to every version the WIT names. | ||
| pub version: Option<VersionReq>, | ||
| } |
There was a problem hiding this comment.
Could we reuse the PackageSpec type instead of defining DependencyKey?
wasm-pkg-tools/crates/wasm-pkg-common/src/package.rs
Lines 83 to 86 in bc0ece4
| /// Checks that override keys parse and that no package is covered by both a bare and a | ||
| /// versioned key. | ||
| /// | ||
| /// Runs when a `wkg.toml` is loaded (see [`validate`](Self::validate)), and again when |
There was a problem hiding this comment.
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html
| /// Runs when a `wkg.toml` is loaded (see [`validate`](Self::validate)), and again when | |
| /// Runs when a `wkg.toml` is loaded (see [`Self::validate`]), and again when |
| /// Runs when a `wkg.toml` is loaded (see [`validate`](Self::validate)), and again when | ||
| /// resolving, since a `Manifest` built directly in Rust code skips the load step. |
There was a problem hiding this comment.
Actually this section should be removed since it implies assumptions about a potential parent scope
| /// Runs when a `wkg.toml` is loaded (see [`validate`](Self::validate)), and again when | |
| /// resolving, since a `Manifest` built directly in Rust code skips the load step. |
mkatychev
left a comment
There was a problem hiding this comment.
@Aditya1404Sal I have been away for the last two weeks so I appreciate the (involuntary!) patience
|
@mkatychev -- no worries at all :) hope you had some quality time! |
| // ```toml | ||
| // [overrides] | ||
| // "my:local@0.1.0" = { "path" = "../local-dep-0.1.0/wit" } | ||
| // "my:local@0.2.0" = { "path" = "../local-dep-0.2.0/wit" } | ||
| // ``` | ||
| let manifest = Manifest { | ||
| overrides: Some(HashMap::from([ | ||
| ( | ||
| "my:local@0.1.0".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, | ||
| }, | ||
| ), | ||
| ])), |
There was a problem hiding this comment.
are we able to use Manifest::from_toml here to roll the comment and concrete object into one?
wasm-pkg-tools/crates/wasm-pkg-core/src/manifest/workspace.rs
Lines 237 to 244 in bc0ece4
| /// Recorded before the dependency itself is added, so that versioned requests for the same | ||
| /// package arriving later are skipped. Only overrides get here: an unversioned WIT import is | ||
| /// a single dependency with no version, not an override covering every version. | ||
| fn record_any_version_override(&mut self, key: &PackageSpec) { | ||
| if key.version.is_none() { | ||
| self.any_version_overrides.insert(key.package.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a little confused as to the limits of this step, can a key override other overrides?
If we should probably forbid having multiple overrides matching to the same value.
EDIT: I realise that override_key_conflicts_are_all_reported_in_a_stable_order touches on this but we should be exiting early without having to resort to asking things such as is_already_superseded and has_superseded_resolution
There was a problem hiding this comment.
It may be better to collect all overrides and ensuring they do not intersect before applying the overrides.
| /// Returns whether resolving `key` also satisfies `other`. A key without a version (`foo:bar`) | ||
| /// covers every version of its package; `foo:bar@0.1.0` covers only itself. | ||
| fn key_supersedes(key: &PackageSpec, other: &PackageSpec) -> bool { | ||
| key.package == other.package && (key.version.is_none() || key.version == other.version) | ||
| } |
There was a problem hiding this comment.
we should probably have this and key_requirement be methods on PackageSpec and expose methods that allow us to test for intersections (similar to https://doc.rust-lang.org/std/collections/struct.HashSet.html#method.intersection) since something like local-dep@* is a range whereas local-dep@=0.1.0 is better thought of as a point.
There was a problem hiding this comment.
We should also be unambiguous about distinguishing a PackageSpec { .., version: Some(..) } where a package has NO version (should not match any version) versus one where it can match ALL versions since they can mean completely different things.
Perhaps a PackageRequirement?
pub struct PackageReq {
pub package: PackageRef,
pub version: VersionReq,
}Though I'm not sure if that is valid either, this definitely feels like a design issue if we're having to reach for things like take_superseded_dependencies.
There was a problem hiding this comment.
- We should ideally know inside
DependencyResolutionif a package requirement has already been resolved locally or not:wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 172 to 179 in bc0ece4
- different resolution requirements likely need to be applied to a local or registry resolution ( forbidding unversioned packages in registries should still apply) but one should be able to tell whether a
LocalResolutionresolved not just to a path but an explicit version (or explicit lack thereof)
Perhaps prepopulating DependencyResolutionMap with a new explicit variant might help: DependencyResolution::Unresolved:
wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 664 to 665 in bc0ece4
Ultimately we want to be able to resolve requirements without worrying about order of application, this means being able to identify overlaps in overrides before matching requirements with resolutions; stated more generically: "we want a dependency requirement to be able to path to only one dependency resolution".
There was a problem hiding this comment.
It may be a good idea to look into how PublishPlan uses acyclic graphs since we want only one requirement -> resolution per requirement (but a resolution can meet more than one requirement):
wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 865 to 871 in bc0ece4
Problem
According to the rules governing wit, there's nothing that actively restricts a scenario in which
a wit world can name more than one version of the same package, say for example when a world exports both
wasmcloud:messaging/handler@0.2.0and@0.3.0in the same component (from where I ran into this limitation wasmCloud/wasmCloud#5459)If there's justification needed as to why this is needed, off the top of my head -- it can help in safe migration
to a new interface rev without a breaking change
or in my use, so that a component can A/B test the new one for regressions
wit-bindgendoes support it by producing versioned bindings (I think that's what they're called)messaging0_2_0andmessaging0_3_0so I don't see why wkg can't have it in the override resolutionback to the problem :
Our
wash buildcommand that builds our components hasWkgFetcherwhich could not resolve the mixed deps and while running the fetch flow, one version was always dropped, and the fetch then failed and cleared the
deps/sub-dir.Change
The resolver is now keyed by
PackageSpec-- a package plus the version requirement it wasrequested under.
[overrides]keys accept an exact version suffix:existing behaviour
A bare key like below
will behave exactly as before.