From fc0fcc911deec84c4ebb36fd1ad3c1179c75eb43 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Feb 2023 15:26:33 -0600 Subject: [PATCH] feat(resolver): `-Zdirect-minimal-versions` This is an alternative to `-Zminimal-versions` as discussed in #5657. The problem with `-Zminimal-versions` is it requires the root most dependencies to verify it and we then percolate that up the stack. This requires a massive level of cooperation to accomplish and so far there have been mixed results with it to the point that cargo's unstable documentation discourages its use. `-Zdirect-minimal-versions` instead only applies this rule to your direct dependencies, allowing anyone in the stack to immediately adopt it, independent of everyone else. Special notes - Living up to the name and the existing design, this ignores yanked crates. This makes sense for `^1.1` version requirements but might look weird for `^1.1.1` version requirements as it could select `1.1.2`. - This will error if an indirect dependency requires a newer version. Your version requirement will need to capture what you use **and** all of you dependencies. An alternative design would have tried to merge the result of minimum versions for direct dependencies and maximum versions for indirect dependencies. This would have been complex and led to weird corner cases, making it harder to predict. I also suspect the value gained would be relatively low as you can't verify that version requirement in any other way. - The error could be improved to call out that this was from minimal versions but I felt getting this out now and starting to collect feedback was more important. --- src/cargo/core/features.rs | 2 + src/cargo/core/resolver/dep_cache.rs | 57 +++++++------ src/cargo/core/resolver/mod.rs | 45 ++++++++-- src/cargo/core/resolver/version_prefs.rs | 22 +++-- src/doc/src/reference/unstable.md | 18 ++++ tests/testsuite/direct_minimal_versions.rs | 98 ++++++++++------------ 6 files changed, 149 insertions(+), 93 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 7df3ea975814..8ef3f01e8013 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -718,6 +718,7 @@ unstable_cli_options!( features: Option> = (HIDDEN), jobserver_per_rustc: bool = (HIDDEN), minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum"), + direct_minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum (direct dependencies only)"), mtime_on_use: bool = ("Configure Cargo to update the mtime of used files"), no_index_update: bool = ("Do not update the registry index even if the cache is outdated"), panic_abort_tests: bool = ("Enable support to run tests with -Cpanic=abort"), @@ -948,6 +949,7 @@ impl CliUnstable { "no-index-update" => self.no_index_update = parse_empty(k, v)?, "avoid-dev-deps" => self.avoid_dev_deps = parse_empty(k, v)?, "minimal-versions" => self.minimal_versions = parse_empty(k, v)?, + "direct-minimal-versions" => self.direct_minimal_versions = parse_empty(k, v)?, "advanced-env" => self.advanced_env = parse_empty(k, v)?, "config-include" => self.config_include = parse_empty(k, v)?, "check-cfg" => { diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index dd86170035e1..171920816ccd 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -96,7 +96,11 @@ impl<'a> RegistryQueryer<'a> { /// any candidates are returned which match an override then the override is /// applied by performing a second query for what the override should /// return. - pub fn query(&mut self, dep: &Dependency) -> Poll>>> { + pub fn query( + &mut self, + dep: &Dependency, + first_minimal_version: bool, + ) -> Poll>>> { if let Some(out) = self.registry_cache.get(dep).cloned() { return out.map(Result::Ok); } @@ -192,14 +196,14 @@ impl<'a> RegistryQueryer<'a> { // When we attempt versions for a package we'll want to do so in a sorted fashion to pick // the "best candidates" first. VersionPreferences implements this notion. - self.version_prefs.sort_summaries( - &mut ret, - if self.minimal_versions { - VersionOrdering::MinimumVersionsFirst - } else { - VersionOrdering::MaximumVersionsFirst - }, - ); + let ordering = if first_minimal_version || self.minimal_versions { + VersionOrdering::MinimumVersionsFirst + } else { + VersionOrdering::MaximumVersionsFirst + }; + let first_version = first_minimal_version; + self.version_prefs + .sort_summaries(&mut ret, ordering, first_version); let out = Poll::Ready(Rc::new(ret)); @@ -218,6 +222,7 @@ impl<'a> RegistryQueryer<'a> { parent: Option, candidate: &Summary, opts: &ResolveOpts, + first_minimal_version: bool, ) -> ActivateResult, Rc>)>> { // if we have calculated a result before, then we can just return it, // as it is a "pure" query of its arguments. @@ -237,22 +242,24 @@ impl<'a> RegistryQueryer<'a> { let mut all_ready = true; let mut deps = deps .into_iter() - .filter_map(|(dep, features)| match self.query(&dep) { - Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))), - Poll::Pending => { - all_ready = false; - // we can ignore Pending deps, resolve will be repeatedly called - // until there are none to ignore - None - } - Poll::Ready(Err(e)) => Some(Err(e).with_context(|| { - format!( - "failed to get `{}` as a dependency of {}", - dep.package_name(), - describe_path_in_context(cx, &candidate.package_id()), - ) - })), - }) + .filter_map( + |(dep, features)| match self.query(&dep, first_minimal_version) { + Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))), + Poll::Pending => { + all_ready = false; + // we can ignore Pending deps, resolve will be repeatedly called + // until there are none to ignore + None + } + Poll::Ready(Err(e)) => Some(Err(e).with_context(|| { + format!( + "failed to get `{}` as a dependency of {}", + dep.package_name(), + describe_path_in_context(cx, &candidate.package_id()), + ) + })), + }, + ) .collect::>>()?; // Attempt to resolve dependencies with fewer candidates before trying diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index b0551891da14..96c425fd755f 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -133,11 +133,21 @@ pub fn resolve( Some(config) => config.cli_unstable().minimal_versions, None => false, }; + let direct_minimal_versions = match config { + Some(config) => config.cli_unstable().direct_minimal_versions, + None => false, + }; let mut registry = RegistryQueryer::new(registry, replacements, version_prefs, minimal_versions); let cx = loop { let cx = Context::new(check_public_visible_dependencies); - let cx = activate_deps_loop(cx, &mut registry, summaries, config)?; + let cx = activate_deps_loop( + cx, + &mut registry, + summaries, + direct_minimal_versions, + config, + )?; if registry.reset_pending() { break cx; } else { @@ -189,6 +199,7 @@ fn activate_deps_loop( mut cx: Context, registry: &mut RegistryQueryer<'_>, summaries: &[(Summary, ResolveOpts)], + direct_minimal_versions: bool, config: Option<&Config>, ) -> CargoResult { let mut backtrack_stack = Vec::new(); @@ -201,7 +212,14 @@ fn activate_deps_loop( // Activate all the initial summaries to kick off some work. for &(ref summary, ref opts) in summaries { debug!("initial activation: {}", summary.package_id()); - let res = activate(&mut cx, registry, None, summary.clone(), opts); + let res = activate( + &mut cx, + registry, + None, + summary.clone(), + direct_minimal_versions, + opts, + ); match res { Ok(Some((frame, _))) => remaining_deps.push(frame), Ok(None) => (), @@ -399,7 +417,15 @@ fn activate_deps_loop( dep.package_name(), candidate.version() ); - let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &opts); + let direct_minimal_version = false; // this is an indirect dependency + let res = activate( + &mut cx, + registry, + Some((&parent, &dep)), + candidate, + direct_minimal_version, + &opts, + ); let successfully_activated = match res { // Success! We've now activated our `candidate` in our context @@ -611,6 +637,7 @@ fn activate( registry: &mut RegistryQueryer<'_>, parent: Option<(&Summary, &Dependency)>, candidate: Summary, + first_minimal_version: bool, opts: &ResolveOpts, ) -> ActivateResult> { let candidate_pid = candidate.package_id(); @@ -662,8 +689,13 @@ fn activate( }; let now = Instant::now(); - let (used_features, deps) = - &*registry.build_deps(cx, parent.map(|p| p.0.package_id()), &candidate, opts)?; + let (used_features, deps) = &*registry.build_deps( + cx, + parent.map(|p| p.0.package_id()), + &candidate, + opts, + first_minimal_version, + )?; // Record what list of features is active for this package. if !used_features.is_empty() { @@ -859,8 +891,9 @@ fn generalize_conflicting( // A dep is equivalent to one of the things it can resolve to. // Thus, if all the things it can resolve to have already ben determined // to be conflicting, then we can just say that we conflict with the parent. + let first_minimal_version = false; if let Some(others) = registry - .query(critical_parents_dep) + .query(critical_parents_dep, first_minimal_version) .expect("an already used dep now error!?") .expect("an already used dep now pending!?") .iter() diff --git a/src/cargo/core/resolver/version_prefs.rs b/src/cargo/core/resolver/version_prefs.rs index 8eb800c40588..73cce5db87ab 100644 --- a/src/cargo/core/resolver/version_prefs.rs +++ b/src/cargo/core/resolver/version_prefs.rs @@ -42,7 +42,12 @@ impl VersionPreferences { /// Sort the given vector of summaries in-place, with all summaries presumed to be for /// the same package. Preferred versions appear first in the result, sorted by /// `version_ordering`, followed by non-preferred versions sorted the same way. - pub fn sort_summaries(&self, summaries: &mut Vec, version_ordering: VersionOrdering) { + pub fn sort_summaries( + &self, + summaries: &mut Vec, + version_ordering: VersionOrdering, + first_version: bool, + ) { let should_prefer = |pkg_id: &PackageId| { self.try_to_use.contains(pkg_id) || self @@ -66,6 +71,9 @@ impl VersionPreferences { _ => previous_cmp, } }); + if first_version { + let _ = summaries.split_off(1); + } } } @@ -115,13 +123,13 @@ mod test { summ("foo", "1.0.9"), ]; - vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string() ); - vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string() @@ -140,13 +148,13 @@ mod test { summ("foo", "1.0.9"), ]; - vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string() ); - vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string() @@ -166,13 +174,13 @@ mod test { summ("foo", "1.0.9"), ]; - vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.2.3, foo/1.1.0, foo/1.2.4, foo/1.0.9".to_string() ); - vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst); + vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false); assert_eq!( describe(&summaries), "foo/1.1.0, foo/1.2.3, foo/1.0.9, foo/1.2.4".to_string() diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 71c839ebb989..7eb5e54e100e 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -67,6 +67,7 @@ Each new feature described below should explain how to use it. * [no-index-update](#no-index-update) — Prevents cargo from updating the index cache. * [avoid-dev-deps](#avoid-dev-deps) — Prevents the resolver from including dev-dependencies during resolution. * [minimal-versions](#minimal-versions) — Forces the resolver to use the lowest compatible version instead of the highest. + * [direct-minimal-versions](#direct-minimal-versions) — Forces the resolver to use the lowest compatible version instead of the highest. * [public-dependency](#public-dependency) — Allows dependencies to be classified as either public or private. * Output behavior * [out-dir](#out-dir) — Adds a directory where artifacts are copied to. @@ -176,6 +177,23 @@ minimum versions that you are actually using. That is, if Cargo.toml says `foo = "1.0.0"` that you don't accidentally depend on features added only in `foo 1.5.0`. +### direct-minimal-versions +* Original Issue: [#4100](https://github.com/rust-lang/cargo/issues/4100) +* Tracking Issue: [#5657](https://github.com/rust-lang/cargo/issues/5657) + +When a `Cargo.lock` file is generated, the `-Z direct-minimal-versions` flag will +resolve the dependencies to the minimum SemVer version that will satisfy the +requirements (instead of the greatest version) for direct dependencies only. + +The intended use-case of this flag is to check, during continuous integration, +that the versions specified in Cargo.toml are a correct reflection of the +minimum versions that you are actually using. That is, if Cargo.toml says +`foo = "1.0.0"` that you don't accidentally depend on features added only in +`foo 1.5.0`. + +Indirect dependencies are resolved as normal so as not to be blocked on their +minimal version validation. + ### out-dir * Original Issue: [#4875](https://github.com/rust-lang/cargo/issues/4875) * Tracking Issue: [#6790](https://github.com/rust-lang/cargo/issues/6790) diff --git a/tests/testsuite/direct_minimal_versions.rs b/tests/testsuite/direct_minimal_versions.rs index b222ce2cd202..0e62d6ce0832 100644 --- a/tests/testsuite/direct_minimal_versions.rs +++ b/tests/testsuite/direct_minimal_versions.rs @@ -26,8 +26,8 @@ fn simple() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("generate-lockfile -Zminimal-versions") - .masquerade_as_nightly_cargo(&["minimal-versions"]) + p.cargo("generate-lockfile -Zdirect-minimal-versions") + .masquerade_as_nightly_cargo(&["direct-minimal-versions"]) .run(); let lock = p.read_lockfile(); @@ -67,24 +67,24 @@ fn mixed_dependencies() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("generate-lockfile -Zminimal-versions") - .masquerade_as_nightly_cargo(&["minimal-versions"]) - .run(); + p.cargo("generate-lockfile -Zdirect-minimal-versions") + .masquerade_as_nightly_cargo(&["direct-minimal-versions"]) + .with_status(101) + .with_stderr( + r#"[UPDATING] [..] +[ERROR] failed to select a version for `dep`. + ... required by package `foo v0.0.1 ([CWD])` +versions that meet the requirements `^1.1` are: 1.1.0 - let lock = p.read_lockfile(); +all possible versions conflict with previously selected packages. - assert!( - !lock.contains("1.0.0"), - "dep incompatible version cannot be present" - ); - assert!( - lock.contains("1.1.0"), - "dep minimal version must be present" - ); - assert!( - !lock.contains("1.2.0"), - "dep maximimal version cannot be present" - ); + previously selected package `dep v1.0.0` + ... which satisfies dependency `dep = "^1.0"` of package `foo v0.0.1 ([CWD])` + +failed to select a version for `dep` which could resolve this conflict +"#, + ) + .run(); } #[cargo_test] @@ -109,8 +109,8 @@ fn yanked() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("generate-lockfile -Zminimal-versions") - .masquerade_as_nightly_cargo(&["minimal-versions"]) + p.cargo("generate-lockfile -Zdirect-minimal-versions") + .masquerade_as_nightly_cargo(&["direct-minimal-versions"]) .run(); let lock = p.read_lockfile(); @@ -157,8 +157,8 @@ fn indirect() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("generate-lockfile -Zminimal-versions") - .masquerade_as_nightly_cargo(&["minimal-versions"]) + p.cargo("generate-lockfile -Zdirect-minimal-versions") + .masquerade_as_nightly_cargo(&["direct-minimal-versions"]) .run(); let lock = p.read_lockfile(); @@ -173,15 +173,15 @@ fn indirect() { ); assert!( !lock.contains("2.0.0"), - "indirect unmatched version cannot be present" + "indirect minimal version cannot be present" ); assert!( - lock.contains("2.1.0"), - "indirect minimal version must be present" + !lock.contains("2.1.0"), + "indirect minimal version cannot be present" ); assert!( - !lock.contains("2.2.0"), - "indirect maximimal version cannot be present" + lock.contains("2.2.0"), + "indirect maximal version must be present" ); } @@ -214,35 +214,23 @@ fn indirect_conflict() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("generate-lockfile -Zminimal-versions") - .masquerade_as_nightly_cargo(&["minimal-versions"]) + p.cargo("generate-lockfile -Zdirect-minimal-versions") + .masquerade_as_nightly_cargo(&["direct-minimal-versions"]) + .with_status(101) .with_stderr( - "\ -[UPDATING] [..] -", - ) - .run(); + r#"[UPDATING] [..] +[ERROR] failed to select a version for `indirect`. + ... required by package `direct v1.0.0` + ... which satisfies dependency `direct = "^1.0"` of package `foo v0.0.1 ([CWD])` +versions that meet the requirements `^2.1` are: 2.2.0, 2.1.0 - let lock = p.read_lockfile(); +all possible versions conflict with previously selected packages. - assert!( - lock.contains("1.0.0"), - "direct minimal version must be present" - ); - assert!( - !lock.contains("1.1.0"), - "direct maximimal version cannot be present" - ); - assert!( - !lock.contains("2.0.0"), - "indirect unmatched version cannot be present" - ); - assert!( - lock.contains("2.1.0"), - "indirect minimal version must be present" - ); - assert!( - !lock.contains("2.2.0"), - "indirect maximimal version cannot be present" - ); + previously selected package `indirect v2.0.0` + ... which satisfies dependency `indirect = "^2.0"` of package `foo v0.0.1 ([CWD])` + +failed to select a version for `indirect` which could resolve this conflict +"#, + ) + .run(); }