Skip to content

Commit

Permalink
Auto merge of #12614 - epage:partial, r=weihanglo
Browse files Browse the repository at this point in the history
feat(pkgid): Allow incomplete versions when unambigious

### What does this PR try to resolve?

This was proposed in #12425 to help sand off some of the rough edges around `cargo update` for its wider use it would be getting.  Its easy to accidentally get duplicate copies packages in a repo and a pain to have to specify the full version when `cargo update -p foo@1` is sufficient to describe it.

Other effects
- profile overrides also supports this since we already allow a spec to match multiple items
- `cargo clean -p foo@...` already ignored the version, so now we also parse and ignore the partial version
- `cargo tree --prune` will now accept partial versions and will match all of them

Parts not effected:
- Replacements
  - Two of the cases were found and we treat it as if the version isn't present which will error, so I think that is correct

### How should we test and review this PR?

This extracts `PartialVersion` from `RustVersion` where `RustVersion` is a more specialized variant, not allowing prerelease or build.

This works by adopting `PartialVersion` into `PackageIdSpec`.  For `PackageIdSpec::query`, this will "just work".

### Additional information
  • Loading branch information
bors committed Sep 16, 2023
2 parents d5336f8 + 82f9bd3 commit da498c8
Show file tree
Hide file tree
Showing 22 changed files with 420 additions and 128 deletions.
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()) {
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#[email protected]",
Expand All @@ -362,11 +377,20 @@ mod tests {
"https://crates.io/foo#[email protected]",
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#[email protected]",
);
ok(
"https://crates.io/foo#[email protected]",
PackageIdSpec {
name: InternedString::new("bar"),
version: Some("1.2".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#[email protected]",
);
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,
},
"[email protected]",
Expand All @@ -389,21 +413,29 @@ mod tests {
"[email protected]",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2.3".to_semver().unwrap()),
version: Some("1.2.3".parse().unwrap()),
url: None,
},
"[email protected]",
);
ok(
"[email protected]",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2".parse().unwrap()),
url: None,
},
"[email protected]",
);
}

#[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("[email protected]").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("[email protected]").unwrap().matches(foo));
assert!(!PackageIdSpec::parse("[email protected]").unwrap().matches(foo));
assert!(PackageIdSpec::parse("[email protected]").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

0 comments on commit da498c8

Please sign in to comment.