Support semver version ranges for dependencies - #311
Conversation
- Impl is in place but more thorough tests needed to validate
| Local dependencies do not have a version requirement — the package at that | ||
| path is used as-is. They cannot be mixed with a remote entry for the same | ||
| package name. |
There was a problem hiding this comment.
Can we keep the removed lines:
| Local dependencies do not have a version requirement — the package at that | |
| path is used as-is. They cannot be mixed with a remote entry for the same | |
| package name. | |
| The `path` field is a relative path from the manifest file to the dependency's | |
| root directory (the directory containing the dependency's `Proto.toml`). | |
| Local dependencies do not have a version requirement — the package at that | |
| path is used as-is. They cannot be mixed with a remote entry for the same | |
| package name. | |
| See [Local Dependencies](../guide/local-dependencies.md) for more information. |
There was a problem hiding this comment.
Good catch. Fixed.
Please check you're happy.
| }; | ||
|
|
||
| // Phase 1: resolve the requirement to a concrete version. | ||
| // Try the lockfile first (avoids a registry round-trip when the lock is fresh). |
There was a problem hiding this comment.
The existing tests/cmd/install/workspace/lockfile/stale/mod.rs test already executes two consecutive installs and verifies the lockfile is reused and extended. However, afaik, it uses exact pins (=0.1.0), so it tests the old Lockfile::get() path with exact version matching, not explicitly covering the new find_satisfying() path with range matching.
Could you add a test for that path please?
For example: run install with ^1.0.0, get 1.2.0 locked, run install again, verify it reuses 1.2.0 from the lockfile without hitting the registry
There was a problem hiding this comment.
I've now added tests/cmd/install/workspace/lockfile/range_pin_reuse/mod.rs to test this path.
| self.validate_compatibility(dependency, existing) | ||
| .wrap_err_with(|| format!("conflicting dependency on {}", package_name))?; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
I think there might be a bug here.
IIUC, when a package is encountered for the second time (via a different path in the dependency tree), the resolver doesn't re-resolve.
For example, given:
- pkg-1 requires lib-a
^1.0.0 - pkg-2 requires lib-a
<=1.2.0 - highest version of lib-a is
1.5.0
If pkg-1 resolves first, the version is 1.5.0.
When pkg-2 tries to resolve, 1.5.0 doesn't satisfy <=1.2.0, so install fails.
However, in this case I'd expect it to resolve to 1.2.0 across the requirements.
If pkg-2 would resolves first, the version would satisfy pkg-1's requirement.
CMIIW, but since DependencyMap is HashMap<PackageName, DependencyManifest>, I believe the dependency iteration order is also non-deterministic. That would make an integration test for the order-dependency inherently flaky.
There was a problem hiding this comment.
It's worth adding a test for these cases.
There was a problem hiding this comment.
Thanks for pointing this out. It's likely that I'll need to change the resolver algorithm away from the current single-pass recursive descent. It will need to keep track of aggregated package/requirement information, then fetch recursive transitives and flag any changes implied on already resolved package/requirements. Then this process would be looped until either 1. all requirements are met or 2. a subset of the requirements are irresolvable.
I'm working on it and will get back to you.
There was a problem hiding this comment.
My apologies for the delay, I've had an extended illness. I'll try to get back to finishing this soon.
There was a problem hiding this comment.
OK, I've made a bunch more changes and added new tests that should resolve this issue. Ready for another review, thanks.
There was a problem hiding this comment.
The range_cascade_resolution and range_downgrade_diamond tests should exercise this now.
Stop relying on HashMap iteration order when materializing the dep Vec. Required for the resolver merge-and-resolve work to produce a stable graph across runs.
…leStale Reshape the error enum to express the cases the upcoming merge-and-resolve algorithm produces. VersionConflict is deleted; validate_version_compatibility is stubbed with Ok(()) until Task 3 removes it entirely. Also renames Offline.version to Offline.requirements: Vec<VersionReq> and updates both construction sites. Temporarily ignores range_compatible_diamond and range_incompatible_diamond — the new resolver behaviour is not yet in place.
The workspace lockfile is a pool keyed by (name, version) that is shared across independently-resolved members, so a pin that fails the merged requirements of one member is indistinguishable from another member's legitimate entry. Treating that as a hard error is a false positive; the resolver re-resolves against the registry instead.
…erging GraphBuilder now collects every requirement edge in a package's dependency graph into a per-package set. Resolution picks the highest version satisfying every accumulated requirement at once, instead of locking in the first encountered requirement's match, and retracts the edges a superseded version contributed. Merging is a within-graph concern: workspace members are resolved independently by design, so the diamond that motivates this change is expressed transitively. pkg1 -> lib-x -> lib-a ^1.0.0 pkg1 -> lib-y -> lib-a <=1.2.0 -> resolves to 1.2.0 DependencyNode::version now carries the exact resolved version so the installer cannot re-resolve upward past the resolver's pick. Cycle detection moves from a mid-descent visiting set (which cannot fire under BFS) to a topological check on the finished graph.
Covers the case where a transitive edge tightens an already-resolved package's requirement set so the previous version no longer fits: the edges that version contributed are retracted, newly-unreachable packages are dropped, and the new version's transitives are walked. Also re-enables the incompatible-diamond test against the NoCompatibleVersion variant, which now reports the full requirement set rather than the first pairwise conflict.
Adds a fixture where a second transitive path tightens a package's requirements past the existing pin, and asserts the resolver re-resolves and rewrites the lockfile rather than failing. Also pins down that the pooled workspace lockfile is unaffected: entries belonging to other members are skipped, not mistaken for stale pins.
range_cascade_resolution exercises retraction end-to-end, but only one level of it: the dropped package contributes no further edges. Add unit tests over retract_contributions_from for the branches it cannot reach — transitive cascade through a dropped package, rescheduling a survivor whose pick no longer fits the shrunken requirement set, and leaving a still-satisfied survivor at its current version.
range_pin_reuse validates that once a package version enters the lockfile, if it still satisfies all range criteria, that version is reused without hitting the registry.
Summary
This addresses and implements #205
^1.0,~1.2.0,>=1.5.0,=1.0.0, etc.), not just=x.y.zlist_versions()andresolve_version()toArtifactory— queries the registry for all available versions and selects the highest one satisfying the requirement;get_latest_version()is reimplemented on top of thisdownload()now takes an explicit&Version— resolution is decoupled from downloading; bothArtifactoryandLocalRegistryreceive the resolved version directlyfind_satisfying()helpers to lockfile types —PackageLockfile,WorkspaceLockfile, and theLockfileenum can now look up the best locked version matching aVersionReq, enabling the installer to skip the registry for already-locked rangesdependency_version_string()fromsrc/registry/mod.rsalong with theVersionNotPinnederror — the old pin-enforcement code is gone entirelydocs/src/reference/semver.md) and the resolver (docs/src/reference/resolver.md); updatedspecifying-dependencies.mdandbuffrs-add.mdWhat a reviewer should know
The resolver remains single-pass (interleaves graph walking and downloading). Version resolution now works as:
Artifactory::resolve_version()to query the registry → download the resolved exact version and cache itConflict detection (incompatible diamond) is handled in
validate_version_compatibility()insrc/resolver.rs— unchanged in structure, now exercised by the new test cases.The
queryaxum feature was enabled inCargo.tomlfor the test mock registry to parse search query parameters.Test plan
cargo buildpasses (requiresprotoc)cargo test --workspacepasses (requiresprotoc+ Git LFS)cargo clippy --all-targets --workspace -- -D warnings -D clippy::allpassescargo fmt --check --allpassescargo test --test e2e installcovers the five new range scenarios