From 3e34dfb5b269d18b8c10384586808b62681cc7d1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 1 Sep 2023 13:30:58 -0500 Subject: [PATCH] feat(spec): Allow partial versions when unambigious This was proposed in #12425 to help improve usability of the existing `cargo update` when dealing with the added workflows. --- src/bin/cargo/commands/remove.rs | 2 +- src/cargo/core/package_id_spec.rs | 67 +++++++++++++++++++++------- src/cargo/ops/cargo_clean.rs | 2 +- src/cargo/util/mod.rs | 2 +- src/cargo/util/semver_ext.rs | 22 +++++++++ src/cargo/util/toml/mod.rs | 4 +- src/doc/src/reference/pkgid-spec.md | 2 + tests/testsuite/clean.rs | 32 +++++++++---- tests/testsuite/pkgid.rs | 19 +++----- tests/testsuite/profile_overrides.rs | 14 +----- 10 files changed, 111 insertions(+), 55 deletions(-) diff --git a/src/bin/cargo/commands/remove.rs b/src/bin/cargo/commands/remove.rs index e8f6ab98e804..7e24df77bfd7 100644 --- a/src/bin/cargo/commands/remove.rs +++ b/src/bin/cargo/commands/remove.rs @@ -285,7 +285,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, }; diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 68561aecc7d7..f49c90b62737 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -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: /// @@ -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: Option, url: Option, } @@ -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::()?), None => None, }; validate_package_name(name, "pkgid", "")?; @@ -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()), } } @@ -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::()?; (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::()?; (InternedString::new(path_name), Some(version)) } } @@ -151,7 +152,12 @@ impl PackageIdSpec { self.name } - pub fn version(&self) -> Option<&Version> { + /// Full `semver::Version`, if present + pub fn version(&self) -> Option { + self.version.as_ref().and_then(|v| v.version()) + } + + pub fn partial_version(&self) -> Option<&PartialVersion> { self.version.as_ref() } @@ -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; } } @@ -322,7 +329,6 @@ mod tests { use super::PackageIdSpec; use crate::core::{PackageId, SourceId}; use crate::util::interning::InternedString; - use crate::util::ToSemver; use url::Url; #[test] @@ -347,16 +353,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", @@ -365,11 +380,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 { @@ -383,7 +407,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", @@ -392,21 +416,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()); } @@ -424,5 +456,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)); } } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index d28f759d7076..e05ffecba954 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -108,7 +108,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { for spec_str in opts.spec.iter() { // Translate the spec to a Package. let spec = PackageIdSpec::parse(spec_str)?; - if spec.version().is_some() { + if spec.partial_version().is_some() { config.shell().warn(&format!( "version qualifier in `-p {}` is ignored, \ cleaning all versions of `{}` found", diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 74fcc96050a5..c3193a741088 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -22,7 +22,7 @@ pub use self::progress::{Progress, ProgressStyle}; pub use self::queue::Queue; pub use self::restricted_names::validate_package_name; pub use self::rustc::Rustc; -pub use self::semver_ext::{OptVersionReq, RustVersion, VersionExt, VersionReqExt}; +pub use self::semver_ext::{OptVersionReq, PartialVersion, RustVersion, VersionExt, VersionReqExt}; pub use self::to_semver::ToSemver; pub use self::vcs::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; pub use self::workspace::{ diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index b03473fb3fa1..1ed12fbf519f 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -164,6 +164,16 @@ pub struct PartialVersion { } impl PartialVersion { + pub fn version(&self) -> Option { + Some(Version { + major: self.major, + minor: self.minor?, + patch: self.patch?, + pre: self.pre.clone().unwrap_or_default(), + build: self.build.clone().unwrap_or_default(), + }) + } + pub fn caret_req(&self) -> VersionReq { VersionReq { comparators: vec![Comparator { @@ -175,6 +185,18 @@ impl PartialVersion { }], } } + + pub fn exact_req(&self) -> VersionReq { + VersionReq { + comparators: vec![Comparator { + op: semver::Op::Exact, + major: self.major, + minor: self.minor, + patch: self.patch, + pre: self.pre.as_ref().cloned().unwrap_or_default(), + }], + } + } } impl From for PartialVersion { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 79f49ff47e79..de92cbf123b7 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2653,8 +2653,8 @@ impl TomlManifest { replacement.unused_keys(), &mut cx.warnings, ); - dep.set_version_req(VersionReq::exact(version)) - .lock_version(version); + dep.set_version_req(VersionReq::exact(&version)) + .lock_version(&version); replace.push((spec, dep)); } Ok(replace) diff --git a/src/doc/src/reference/pkgid-spec.md b/src/doc/src/reference/pkgid-spec.md index bf2e15c8c1f7..7f20973b5092 100644 --- a/src/doc/src/reference/pkgid-spec.md +++ b/src/doc/src/reference/pkgid-spec.md @@ -24,6 +24,7 @@ The formal grammar for a Package Id Specification is: spec := pkgname | proto "://" hostname-and-path [ "#" ( pkgname | semver ) ] pkgname := name [ ("@" | ":" ) semver ] +semver := digits [ "." digits [ "." digits [ "-" prerelease ] [ "+" build ]]] proto := "http" | "git" | ... ``` @@ -40,6 +41,7 @@ The following are references to the `regex` package on `crates.io`: | Spec | Name | Version | |:------------------------------------------------------------|:-------:|:-------:| | `regex` | `regex` | `*` | +| `regex@1.4` | `regex` | `1.4.*` | | `regex@1.4.3` | `regex` | `1.4.3` | | `https://github.com/rust-lang/crates.io-index#regex` | `regex` | `*` | | `https://github.com/rust-lang/crates.io-index#regex@1.4.3` | `regex` | `1.4.3` | diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index bf2df9cc60ef..26cc11a4a2ad 100644 --- a/tests/testsuite/clean.rs +++ b/tests/testsuite/clean.rs @@ -659,13 +659,21 @@ error: package ID specification `baz` did not match any packages .run(); p.cargo("clean -p bar:0.1") - .with_status(101) .with_stderr( - "\ -error: cannot parse '0.1' as a SemVer version -", + "warning: version qualifier in `-p bar:0.1` is ignored, \ + cleaning all versions of `bar` found", ) .run(); + let mut walker = walkdir::WalkDir::new(p.build_dir()) + .into_iter() + .filter_map(|e| e.ok()) + .filter(|e| { + let n = e.file_name().to_str().unwrap(); + n.starts_with("bar") || n.starts_with("libbar") + }); + if let Some(e) = walker.next() { + panic!("{:?} was not cleaned", e.path()); + } } #[cargo_test] @@ -705,13 +713,21 @@ error: package ID specification `baz` did not match any packages .run(); p.cargo("clean -p bar:0") - .with_status(101) .with_stderr( - "\ -error: cannot parse '0' as a SemVer version -", + "warning: version qualifier in `-p bar:0` is ignored, \ + cleaning all versions of `bar` found", ) .run(); + let mut walker = walkdir::WalkDir::new(p.build_dir()) + .into_iter() + .filter_map(|e| e.ok()) + .filter(|e| { + let n = e.file_name().to_str().unwrap(); + n.starts_with("bar") || n.starts_with("libbar") + }); + if let Some(e) = walker.next() { + panic!("{:?} was not cleaned", e.path()); + } } #[cargo_test] diff --git a/tests/testsuite/pkgid.rs b/tests/testsuite/pkgid.rs index 1857f43b1857..3cac7b9692ea 100644 --- a/tests/testsuite/pkgid.rs +++ b/tests/testsuite/pkgid.rs @@ -151,25 +151,20 @@ fn multiple_versions() { .with_status(101) .with_stderr( "\ -error: invalid package ID specification: `two-ver@0` - -Did you mean `two-ver`? - -Caused by: - cannot parse '0' as a SemVer version +error: There are multiple `two-ver` packages in your project, and the specification `two-ver@0` is ambiguous. +Please re-run this command with `-p ` where `` is one of the following: + two-ver@0.1.0 + two-ver@0.2.0 ", ) .run(); // Incomplete version. p.cargo("pkgid two-ver@0.2") - .with_status(101) - .with_stderr( + .with_status(0) + .with_stdout( "\ -error: invalid package ID specification: `two-ver@0.2` - -Caused by: - cannot parse '0.2' as a SemVer version +https://github.com/rust-lang/crates.io-index#two-ver@0.2.0 ", ) .run(); diff --git a/tests/testsuite/profile_overrides.rs b/tests/testsuite/profile_overrides.rs index 34e53bf0c4ce..65d9f3b51925 100644 --- a/tests/testsuite/profile_overrides.rs +++ b/tests/testsuite/profile_overrides.rs @@ -317,19 +317,7 @@ fn profile_override_spec_with_partial_version() { .build(); p.cargo("check -v") - .with_status(101) - .with_stderr_contains( - "\ -error: failed to parse manifest at `[CWD]/Cargo.toml` - -Caused by: - TOML parse error at line 9, column 34 - | - 9 | [profile.dev.package.\"bar:0.5\"] - | ^^^^^^^^^ - cannot parse '0.5' as a SemVer version -", - ) + .with_stderr_contains("[RUNNING] `rustc [..]bar/src/lib.rs [..] -C codegen-units=2 [..]") .run(); }