Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinguish lockfile version req from normal dep in resolver error message #9847

Merged
merged 5 commits into from
Oct 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/cargo/core/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ impl Dependency {
/// Locks this dependency to depending on the specified package ID.
pub fn lock_to(&mut self, id: PackageId) -> &mut Dependency {
assert_eq!(self.inner.source_id, id.source_id());
assert!(self.inner.req.matches(id.version()));
trace!(
"locking dep from `{}` with `{}` at {} to {}",
self.package_name(),
Expand All @@ -329,7 +328,7 @@ impl Dependency {
id
);
let me = Rc::make_mut(&mut self.inner);
me.req = OptVersionReq::exact(id.version());
me.req.lock_to(id.version());

// Only update the `precise` of this source to preserve other
// information about dependency's source which may not otherwise be
Expand All @@ -340,10 +339,20 @@ impl Dependency {
self
}

/// Returns `true` if this is a "locked" dependency, basically whether it has
/// an exact version req.
/// Locks this dependency to a specified version.
///
/// Mainly used in dependency patching like `[patch]` or `[replace]`, which
/// doesn't need to lock the entire dependency to a specific [`PackageId`].
pub fn lock_version(&mut self, version: &semver::Version) -> &mut Dependency {
let me = Rc::make_mut(&mut self.inner);
me.req.lock_to(version);
self
}

/// Returns `true` if this is a "locked" dependency. Basically a locked
/// dependency has an exact version req, but not vice versa.
pub fn is_locked(&self) -> bool {
self.inner.req.is_exact()
self.inner.req.is_locked()
}

/// Returns `false` if the dependency is only used to build the local package.
Expand Down
6 changes: 2 additions & 4 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ use crate::core::{Dependency, PackageId, Source, SourceId, SourceMap, Summary};
use crate::sources::config::SourceConfigMap;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::{profile, CanonicalUrl, Config, VersionReqExt};
use crate::util::{profile, CanonicalUrl, Config};
use anyhow::{bail, Context as _};
use log::{debug, trace};
use semver::VersionReq;
use url::Url;

/// Source of information about a group of packages.
Expand Down Expand Up @@ -765,8 +764,7 @@ fn lock(
if locked.source_id() == dep.source_id() {
dep.lock_to(locked);
} else {
let req = VersionReq::exact(locked.version());
dep.set_version_req(req);
dep.lock_version(locked.version());
}
return dep;
}
Expand Down
10 changes: 8 additions & 2 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,10 +387,16 @@ pub(crate) fn describe_path<'a>(
} else {
dep.name_in_toml().to_string()
};
let locked_version = dep
.version_req()
.locked_version()
.map(|v| format!("(locked to {}) ", v))
.unwrap_or_default();

write!(
dep_path_desc,
"\n ... which satisfies {}dependency `{}` of package `{}`",
source_kind, requirement, pkg
"\n ... which satisfies {}dependency `{}` {}of package `{}`",
source_kind, requirement, locked_version, pkg
)
.unwrap();
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ fn installed_exact_package<T>(
where
T: Source,
{
if !dep.is_locked() {
if !dep.version_req().is_exact() {
// If the version isn't exact, we may need to update the registry and look for a newer
// version - we can't know if the package is installed without doing so.
return Ok(None);
Expand Down
76 changes: 73 additions & 3 deletions src/cargo/util/semver_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use std::fmt::{self, Display};
pub enum OptVersionReq {
Any,
Req(VersionReq),
/// The exact locked version and the original version requirement.
Locked(Version, VersionReq),
}

pub trait VersionExt {
Expand Down Expand Up @@ -49,22 +51,53 @@ impl OptVersionReq {
cmp.op == Op::Exact && cmp.minor.is_some() && cmp.patch.is_some()
}
}
OptVersionReq::Locked(..) => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the is_exact method still used anywhere? If sow why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, for cargo install to decide whether to skip registry update or not. Though I found that some other duplicated logic laying there. 😂
https://github.com/rust-lang/cargo/pull/9847/files#diff-085d4f28bd6e49633caa467300082e94527f840dd487242b13bad2882a25451fR678

}
}

pub fn lock_to(&mut self, version: &Version) {
assert!(self.matches(version), "cannot lock {} to {}", self, version);
use OptVersionReq::*;
let version = version.clone();
*self = match self {
Any => Locked(version, VersionReq::STAR),
Req(req) => Locked(version, req.clone()),
Locked(_, req) => Locked(version, req.clone()),
};
}

pub fn is_locked(&self) -> bool {
matches!(self, OptVersionReq::Locked(..))
}

/// Gets the version to which this req is locked, if any.
pub fn locked_version(&self) -> Option<&Version> {
match self {
OptVersionReq::Locked(version, _) => Some(version),
_ => None,
}
}

pub fn matches(&self, version: &Version) -> bool {
match self {
OptVersionReq::Any => true,
OptVersionReq::Req(req) => req.matches(version),
OptVersionReq::Locked(v, _) => {
v.major == version.major
&& v.minor == version.minor
&& v.patch == version.patch
&& v.pre == version.pre
}
}
}
}

impl Display for OptVersionReq {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
OptVersionReq::Any => formatter.write_str("*"),
OptVersionReq::Req(req) => Display::fmt(req, formatter),
OptVersionReq::Any => f.write_str("*"),
OptVersionReq::Req(req) => Display::fmt(req, f),
OptVersionReq::Locked(_, req) => Display::fmt(req, f),
}
}
}
Expand All @@ -74,3 +107,40 @@ impl From<VersionReq> for OptVersionReq {
OptVersionReq::Req(req)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn locked_has_the_same_with_exact() {
fn test_versions(target_ver: &str, vers: &[&str]) {
let ver = Version::parse(target_ver).unwrap();
let exact = OptVersionReq::exact(&ver);
let mut locked = exact.clone();
locked.lock_to(&ver);
for v in vers {
let v = Version::parse(v).unwrap();
assert_eq!(exact.matches(&v), locked.matches(&v));
}
}

test_versions(
"1.0.0",
&["1.0.0", "1.0.1", "0.9.9", "0.10.0", "0.1.0", "1.0.0-pre"],
);
test_versions("0.9.0", &["0.9.0", "0.9.1", "1.9.0", "0.0.9", "0.9.0-pre"]);
test_versions("0.0.2", &["0.0.2", "0.0.1", "0.0.3", "0.0.2-pre"]);
test_versions(
"0.1.0-beta2.a",
&[
"0.1.0-beta2.a",
"0.9.1",
"0.1.0",
"0.1.1-beta2.a",
"0.1.0-beta2",
],
);
test_versions("0.1.0+meta", &["0.1.0", "0.1.0+meta", "0.1.0+any"]);
}
}
19 changes: 9 additions & 10 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1589,16 +1589,15 @@ impl TomlManifest {
}

let mut dep = replacement.to_dependency(spec.name().as_str(), cx, None)?;
{
let version = spec.version().ok_or_else(|| {
anyhow!(
"replacements must specify a version \
to replace, but `{}` does not",
spec
)
})?;
dep.set_version_req(VersionReq::exact(version));
}
let version = spec.version().ok_or_else(|| {
anyhow!(
"replacements must specify a version \
to replace, but `{}` does not",
spec
)
})?;
dep.set_version_req(VersionReq::exact(version))
.lock_version(version);
replace.push((spec, dep));
}
Ok(replace)
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ fn links_duplicates_old_registry() {
but a native library can be linked only once

package `bar v0.1.0`
... which satisfies dependency `bar = \"=0.1.0\"` of package `foo v0.1.0 ([..]foo)`
... which satisfies dependency `bar = \"^0.1\"` (locked to 0.1.0) of package `foo v0.1.0 ([..]foo)`
links to native library `a`

package `foo v0.1.0 ([..]foo)`
Expand Down