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

feat(pkgid): Allow incomplete versions when unambigious #12614

Merged
merged 9 commits into from
Sep 16, 2023
10 changes: 5 additions & 5 deletions crates/resolver-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use cargo::core::Resolve;
use cargo::core::{Dependency, PackageId, Registry, Summary};
use cargo::core::{GitReference, SourceId};
use cargo::sources::source::QueryKind;
use cargo::util::{CargoResult, Config, Graph, IntoUrl, PartialVersion};
use cargo::util::{CargoResult, Config, Graph, IntoUrl, RustVersion};

use proptest::collection::{btree_map, vec};
use proptest::prelude::*;
Expand Down Expand Up @@ -185,7 +185,7 @@ pub fn resolve_with_config_raw(
deps,
&BTreeMap::new(),
None::<&String>,
None::<PartialVersion>,
None::<RustVersion>,
)
.unwrap();
let opts = ResolveOpts::everything();
Expand Down Expand Up @@ -588,7 +588,7 @@ pub fn pkg_dep<T: ToPkgId>(name: T, dep: Vec<Dependency>) -> Summary {
dep,
&BTreeMap::new(),
link,
None::<PartialVersion>,
None::<RustVersion>,
)
.unwrap()
}
Expand Down Expand Up @@ -616,7 +616,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary {
Vec::new(),
&BTreeMap::new(),
link,
None::<PartialVersion>,
None::<RustVersion>,
)
.unwrap()
}
Expand All @@ -630,7 +630,7 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary {
deps,
&BTreeMap::new(),
sum.links().map(|a| a.as_str()),
None::<PartialVersion>,
None::<RustVersion>,
)
.unwrap()
}
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ fn spec_has_match(
}

let version_matches = match (spec.version(), dep.version()) {
(Some(v), Some(vq)) => semver::VersionReq::parse(vq)?.matches(v),
(Some(v), Some(vq)) => semver::VersionReq::parse(vq)?.matches(&v),
(Some(_), None) => false,
(None, None | Some(_)) => true,
};
Expand Down
12 changes: 6 additions & 6 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::core::{Edition, Feature, Features, WorkspaceConfig};
use crate::util::errors::*;
use crate::util::interning::InternedString;
use crate::util::toml::{TomlManifest, TomlProfiles};
use crate::util::{short_hash, Config, Filesystem, PartialVersion};
use crate::util::{short_hash, Config, Filesystem, RustVersion};

pub enum EitherManifest {
Real(Manifest),
Expand Down Expand Up @@ -58,7 +58,7 @@ pub struct Manifest {
original: Rc<TomlManifest>,
unstable_features: Features,
edition: Edition,
rust_version: Option<PartialVersion>,
rust_version: Option<RustVersion>,
im_a_teapot: Option<bool>,
default_run: Option<String>,
metabuild: Option<Vec<String>>,
Expand Down Expand Up @@ -112,7 +112,7 @@ pub struct ManifestMetadata {
pub documentation: Option<String>, // URL
pub badges: BTreeMap<String, BTreeMap<String, String>>,
pub links: Option<String>,
pub rust_version: Option<PartialVersion>,
pub rust_version: Option<RustVersion>,
}

#[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
Expand Down Expand Up @@ -401,7 +401,7 @@ impl Manifest {
workspace: WorkspaceConfig,
unstable_features: Features,
edition: Edition,
rust_version: Option<PartialVersion>,
rust_version: Option<RustVersion>,
im_a_teapot: Option<bool>,
default_run: Option<String>,
original: Rc<TomlManifest>,
Expand Down Expand Up @@ -570,8 +570,8 @@ impl Manifest {
self.edition
}

pub fn rust_version(&self) -> Option<PartialVersion> {
self.rust_version
pub fn rust_version(&self) -> Option<&RustVersion> {
self.rust_version.as_ref()
}

pub fn custom_metadata(&self) -> Option<&toml::Value> {
Expand Down
8 changes: 4 additions & 4 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::util::network::http::http_handle_and_timeout;
use crate::util::network::http::HttpTimeout;
use crate::util::network::retry::{Retry, RetryResult};
use crate::util::network::sleep::SleepTracker;
use crate::util::PartialVersion;
use crate::util::RustVersion;
use crate::util::{self, internal, Config, Progress, ProgressStyle};

pub const MANIFEST_PREAMBLE: &str = "\
Expand Down Expand Up @@ -104,7 +104,7 @@ pub struct SerializedPackage {
#[serde(skip_serializing_if = "Option::is_none")]
metabuild: Option<Vec<String>>,
default_run: Option<String>,
rust_version: Option<PartialVersion>,
rust_version: Option<RustVersion>,
}

impl Package {
Expand Down Expand Up @@ -178,7 +178,7 @@ impl Package {
self.targets().iter().any(|target| target.proc_macro())
}
/// Gets the package's minimum Rust version.
pub fn rust_version(&self) -> Option<PartialVersion> {
pub fn rust_version(&self) -> Option<&RustVersion> {
self.manifest().rust_version()
}

Expand Down Expand Up @@ -263,7 +263,7 @@ impl Package {
metabuild: self.manifest().metabuild().cloned(),
publish: self.publish().as_ref().cloned(),
default_run: self.manifest().default_run().map(|s| s.to_owned()),
rust_version: self.rust_version(),
rust_version: self.rust_version().cloned(),
}
}
}
Expand Down
67 changes: 50 additions & 17 deletions src/cargo/core/package_id_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ use crate::core::PackageId;
use crate::util::edit_distance;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::{validate_package_name, IntoUrl, ToSemver};
use crate::util::PartialVersion;
use crate::util::{validate_package_name, IntoUrl};

/// Some or all of the data required to identify a package:
///
Expand All @@ -24,7 +25,7 @@ use crate::util::{validate_package_name, IntoUrl, ToSemver};
#[derive(Clone, PartialEq, Eq, Debug, Hash, Ord, PartialOrd)]
pub struct PackageIdSpec {
name: InternedString,
version: Option<Version>,
version: Option<PartialVersion>,
url: Option<Url>,
}

Expand Down Expand Up @@ -70,7 +71,7 @@ impl PackageIdSpec {
let mut parts = spec.splitn(2, [':', '@']);
let name = parts.next().unwrap();
let version = match parts.next() {
Some(version) => Some(version.to_semver()?),
Some(version) => Some(version.parse::<PartialVersion>()?),
None => None,
};
validate_package_name(name, "pkgid", "")?;
Expand All @@ -94,12 +95,12 @@ impl PackageIdSpec {
spec.query(i)
}

/// Convert a `PackageId` to a `PackageIdSpec`, which will have both the `Version` and `Url`
/// Convert a `PackageId` to a `PackageIdSpec`, which will have both the `PartialVersion` and `Url`
/// fields filled in.
pub fn from_package_id(package_id: PackageId) -> PackageIdSpec {
PackageIdSpec {
name: package_id.name(),
version: Some(package_id.version().clone()),
version: Some(package_id.version().clone().into()),
url: Some(package_id.source_id().url().clone()),
}
}
Expand All @@ -125,14 +126,14 @@ impl PackageIdSpec {
match frag {
Some(fragment) => match fragment.split_once([':', '@']) {
Some((name, part)) => {
let version = part.to_semver()?;
let version = part.parse::<PartialVersion>()?;
(InternedString::new(name), Some(version))
}
None => {
if fragment.chars().next().unwrap().is_alphabetic() {
(InternedString::new(&fragment), None)
} else {
let version = fragment.to_semver()?;
let version = fragment.parse::<PartialVersion>()?;
(InternedString::new(path_name), Some(version))
}
}
Expand All @@ -151,7 +152,12 @@ impl PackageIdSpec {
self.name
}

pub fn version(&self) -> Option<&Version> {
/// Full `semver::Version`, if present
pub fn version(&self) -> Option<Version> {
self.version.as_ref().and_then(|v| v.version())
}

pub fn partial_version(&self) -> Option<&PartialVersion> {
self.version.as_ref()
}

Expand All @@ -170,7 +176,8 @@ impl PackageIdSpec {
}

if let Some(ref v) = self.version {
if v != package_id.version() {
let req = v.exact_req();
if !req.matches(package_id.version()) {
Comment on lines +179 to +180
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes us to no longer require the build field matches which causes problems. Working on a fix which we'll need to backport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #12806

return false;
}
}
Expand Down Expand Up @@ -319,7 +326,6 @@ mod tests {
use super::PackageIdSpec;
use crate::core::{PackageId, SourceId};
use crate::util::interning::InternedString;
use crate::util::ToSemver;
use url::Url;

#[test]
Expand All @@ -344,16 +350,25 @@ mod tests {
"https://crates.io/foo#1.2.3",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#1.2.3",
);
ok(
"https://crates.io/foo#1.2",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#1.2",
);
ok(
"https://crates.io/foo#bar:1.2.3",
PackageIdSpec {
name: InternedString::new("bar"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#bar@1.2.3",
Expand All @@ -362,11 +377,20 @@ mod tests {
"https://crates.io/foo#bar@1.2.3",
PackageIdSpec {
name: InternedString::new("bar"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#bar@1.2.3",
);
ok(
"https://crates.io/foo#bar@1.2",
PackageIdSpec {
name: InternedString::new("bar"),
version: Some("1.2".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#bar@1.2",
);
ok(
"foo",
PackageIdSpec {
Expand All @@ -380,7 +404,7 @@ mod tests {
"foo:1.2.3",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: None,
},
"foo@1.2.3",
Expand All @@ -389,21 +413,29 @@ mod tests {
"foo@1.2.3",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: None,
},
"foo@1.2.3",
);
ok(
"foo@1.2",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2".parse().unwrap()),
url: None,
},
"foo@1.2",
);
}

#[test]
fn bad_parsing() {
assert!(PackageIdSpec::parse("baz:").is_err());
assert!(PackageIdSpec::parse("baz:*").is_err());
assert!(PackageIdSpec::parse("baz:1.0").is_err());
assert!(PackageIdSpec::parse("baz@").is_err());
assert!(PackageIdSpec::parse("baz@*").is_err());
assert!(PackageIdSpec::parse("baz@1.0").is_err());
assert!(PackageIdSpec::parse("baz@^1.0").is_err());
assert!(PackageIdSpec::parse("https://baz:1.0").is_err());
assert!(PackageIdSpec::parse("https://#baz:1.0").is_err());
}
Expand All @@ -421,5 +453,6 @@ mod tests {
assert!(!PackageIdSpec::parse("foo:1.2.2").unwrap().matches(foo));
assert!(PackageIdSpec::parse("foo@1.2.3").unwrap().matches(foo));
assert!(!PackageIdSpec::parse("foo@1.2.2").unwrap().matches(foo));
assert!(PackageIdSpec::parse("foo@1.2").unwrap().matches(foo));
}
}
11 changes: 6 additions & 5 deletions src/cargo/core/resolver/dep_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry,
use crate::sources::source::QueryKind;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::PartialVersion;
use crate::util::RustVersion;

use anyhow::Context as _;
use std::collections::{BTreeSet, HashMap, HashSet};
Expand All @@ -36,7 +36,7 @@ pub struct RegistryQueryer<'a> {
/// versions first. That allows `cargo update -Z minimal-versions` which will
/// specify minimum dependency versions to be used.
minimal_versions: bool,
max_rust_version: Option<PartialVersion>,
max_rust_version: Option<RustVersion>,
/// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_minimal_version`)
registry_cache: HashMap<(Dependency, bool), Poll<Rc<Vec<Summary>>>>,
/// a cache of `Dependency`s that are required for a `Summary`
Expand All @@ -58,14 +58,14 @@ impl<'a> RegistryQueryer<'a> {
replacements: &'a [(PackageIdSpec, Dependency)],
version_prefs: &'a VersionPreferences,
minimal_versions: bool,
max_rust_version: Option<PartialVersion>,
max_rust_version: Option<&RustVersion>,
) -> Self {
RegistryQueryer {
registry,
replacements,
version_prefs,
minimal_versions,
max_rust_version,
max_rust_version: max_rust_version.cloned(),
registry_cache: HashMap::new(),
summary_cache: HashMap::new(),
used_replacements: HashMap::new(),
Expand Down Expand Up @@ -115,7 +115,8 @@ impl<'a> RegistryQueryer<'a> {

let mut ret = Vec::new();
let ready = self.registry.query(dep, QueryKind::Exact, &mut |s| {
if self.max_rust_version.is_none() || s.rust_version() <= self.max_rust_version {
if self.max_rust_version.is_none() || s.rust_version() <= self.max_rust_version.as_ref()
{
ret.push(s);
}
})?;
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ use crate::util::config::Config;
use crate::util::errors::CargoResult;
use crate::util::network::PollExt;
use crate::util::profile;
use crate::util::PartialVersion;
use crate::util::RustVersion;

use self::context::Context;
use self::dep_cache::RegistryQueryer;
Expand Down Expand Up @@ -139,7 +139,7 @@ pub fn resolve(
version_prefs: &VersionPreferences,
config: Option<&Config>,
check_public_visible_dependencies: bool,
mut max_rust_version: Option<PartialVersion>,
mut max_rust_version: Option<&RustVersion>,
) -> CargoResult<Resolve> {
let _p = profile::start("resolving");
let minimal_versions = match config {
Expand Down
Loading