From 8dd37c1d2225920a06f57296c59e1a2793d63f1b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Aug 2024 10:03:31 -0500 Subject: [PATCH 1/4] refactor(schema): Pull out test helpers for reuse --- .../src/core/package_id_spec.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index 72d72149e2a..2df7a0c9305 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -323,18 +323,30 @@ mod tests { use crate::core::{GitReference, SourceKind}; use url::Url; + #[track_caller] + fn ok(spec: &str, expected: PackageIdSpec, expected_rendered: &str) { + let parsed = PackageIdSpec::parse(spec).unwrap(); + assert_eq!(parsed, expected); + let rendered = parsed.to_string(); + assert_eq!(rendered, expected_rendered); + let reparsed = PackageIdSpec::parse(&rendered).unwrap(); + assert_eq!(reparsed, expected); + } + + macro_rules! err { + ($spec:expr, $expected:pat) => { + let err = PackageIdSpec::parse($spec).unwrap_err(); + let kind = err.0; + assert!( + matches!(kind, $expected), + "`{}` parse error mismatch, got {kind:?}", + $spec + ); + }; + } + #[test] fn good_parsing() { - #[track_caller] - fn ok(spec: &str, expected: PackageIdSpec, expected_rendered: &str) { - let parsed = PackageIdSpec::parse(spec).unwrap(); - assert_eq!(parsed, expected); - let rendered = parsed.to_string(); - assert_eq!(rendered, expected_rendered); - let reparsed = PackageIdSpec::parse(&rendered).unwrap(); - assert_eq!(reparsed, expected); - } - ok( "https://crates.io/foo", PackageIdSpec { @@ -603,18 +615,6 @@ mod tests { #[test] fn bad_parsing() { - macro_rules! err { - ($spec:expr, $expected:pat) => { - let err = PackageIdSpec::parse($spec).unwrap_err(); - let kind = err.0; - assert!( - matches!(kind, $expected), - "`{}` parse error mismatch, got {kind:?}", - $spec - ); - }; - } - err!("baz:", ErrorKind::PartialVersion(_)); err!("baz:*", ErrorKind::PartialVersion(_)); err!("baz@", ErrorKind::PartialVersion(_)); From 00604fa152ec11e5eada3cf92382e2888d35ba51 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Aug 2024 10:00:25 -0500 Subject: [PATCH 2/4] test(pkgid): Show existing pkgid behavior --- .../src/core/package_id_spec.rs | 55 ++++++++++ tests/testsuite/open_namespaces.rs | 100 ++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index 2df7a0c9305..ea3a99bf1ab 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -437,6 +437,7 @@ mod tests { }, "foo", ); + err!("foo::bar", ErrorKind::PartialVersion(_)); ok( "foo:1.2.3", PackageIdSpec { @@ -447,6 +448,7 @@ mod tests { }, "foo@1.2.3", ); + err!("foo::bar:1.2.3", ErrorKind::PartialVersion(_)); ok( "foo@1.2.3", PackageIdSpec { @@ -457,6 +459,7 @@ mod tests { }, "foo@1.2.3", ); + err!("foo::bar@1.2.3", ErrorKind::PartialVersion(_)); ok( "foo@1.2", PackageIdSpec { @@ -591,6 +594,16 @@ mod tests { }, "file:///path/to/my/project/foo", ); + ok( + "file:///path/to/my/project/foo::bar", + PackageIdSpec { + name: String::from("foo::bar"), + version: None, + url: Some(Url::parse("file:///path/to/my/project/foo::bar").unwrap()), + kind: None, + }, + "file:///path/to/my/project/foo::bar", + ); ok( "file:///path/to/my/project/foo#1.1.8", PackageIdSpec { @@ -611,6 +624,48 @@ mod tests { }, "path+file:///path/to/my/project/foo#1.1.8", ); + ok( + "path+file:///path/to/my/project/foo#bar", + PackageIdSpec { + name: String::from("bar"), + version: None, + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, + "path+file:///path/to/my/project/foo#bar", + ); + err!( + "path+file:///path/to/my/project/foo#foo::bar", + ErrorKind::PartialVersion(_) + ); + ok( + "path+file:///path/to/my/project/foo#bar:1.1.8", + PackageIdSpec { + name: String::from("bar"), + version: Some("1.1.8".parse().unwrap()), + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, + "path+file:///path/to/my/project/foo#bar@1.1.8", + ); + err!( + "path+file:///path/to/my/project/foo#foo::bar:1.1.8", + ErrorKind::PartialVersion(_) + ); + ok( + "path+file:///path/to/my/project/foo#bar@1.1.8", + PackageIdSpec { + name: String::from("bar"), + version: Some("1.1.8".parse().unwrap()), + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, + "path+file:///path/to/my/project/foo#bar@1.1.8", + ); + err!( + "path+file:///path/to/my/project/foo#foo::bar@1.1.8", + ErrorKind::PartialVersion(_) + ); } #[test] diff --git a/tests/testsuite/open_namespaces.rs b/tests/testsuite/open_namespaces.rs index bc4308d1340..c9b62579e77 100644 --- a/tests/testsuite/open_namespaces.rs +++ b/tests/testsuite/open_namespaces.rs @@ -327,6 +327,106 @@ fn main() {} .run(); } +#[cargo_test] +fn generate_pkgid_with_namespace() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["open-namespaces"] + + [package] + name = "foo::bar" + version = "0.0.1" + edition = "2015" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile") + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .run(); + p.cargo("pkgid") + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .with_stdout_data(str![[r#" +path+[ROOTURL]/foo#foo::bar@0.0.1 + +"#]]) + .with_stderr_data("") + .run() +} + +#[cargo_test] +fn update_spec_accepts_namespaced_name() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["open-namespaces"] + + [package] + name = "foo::bar" + version = "0.0.1" + edition = "2015" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile") + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .run(); + p.cargo("update foo::bar") + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .with_status(101) + .with_stdout_data(str![""]) + .with_stderr_data(str![[r#" +[ERROR] invalid package ID specification: `foo::bar` + + Did you mean `foo::bar`? + +Caused by: + expected a version like "1.32" + +"#]]) + .run() +} + +#[cargo_test] +fn update_spec_accepts_namespaced_pkgid() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["open-namespaces"] + + [package] + name = "foo::bar" + version = "0.0.1" + edition = "2015" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile") + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .run(); + p.cargo(&format!("update path+{}#foo::bar@0.0.1", p.url())) + .masquerade_as_nightly_cargo(&["open-namespaces"]) + .with_status(101) + .with_stdout_data(str![""]) + .with_stderr_data(str![[r#" +[ERROR] invalid package ID specification: `path+[ROOTURL]/foo#foo::bar@0.0.1` + +Caused by: + expected a version like "1.32" + +"#]]) + .run() +} + #[cargo_test] #[cfg(unix)] // until we get proper packaging support fn publish_namespaced() { From c81e82dffb04c77cb2a42432a9b1298236e11ea0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Aug 2024 10:19:13 -0500 Subject: [PATCH 3/4] refactor(pkgid): Pull out spec parsing --- .../src/core/package_id_spec.rs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index ea3a99bf1ab..c1ead804aaf 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -94,13 +94,8 @@ impl PackageIdSpec { .into()); } } - let mut parts = spec.splitn(2, [':', '@']); - let name = parts.next().unwrap(); - let version = match parts.next() { - Some(version) => Some(version.parse::()?), - None => None, - }; - PackageName::new(name)?; + let (name, version) = parse_spec(spec)?.unwrap_or_else(|| (spec.to_owned(), None)); + PackageName::new(&name)?; Ok(PackageIdSpec { name: String::from(name), version, @@ -161,11 +156,8 @@ impl PackageIdSpec { return Err(ErrorKind::MissingUrlPath(url).into()); }; match frag { - Some(fragment) => match fragment.split_once([':', '@']) { - Some((name, part)) => { - let version = part.parse::()?; - (String::from(name), Some(version)) - } + Some(fragment) => match parse_spec(&fragment)? { + Some((name, ver)) => (name, ver), None => { if fragment.chars().next().unwrap().is_alphabetic() { (String::from(fragment.as_str()), None) @@ -217,6 +209,15 @@ impl PackageIdSpec { } } +fn parse_spec(spec: &str) -> Result)>> { + let Some((name, ver)) = spec.split_once([':', '@']) else { + return Ok(None); + }; + let name = name.to_owned(); + let ver = ver.parse::()?; + Ok(Some((name, Some(ver)))) +} + fn strip_url_protocol(url: &Url) -> Url { // Ridiculous hoop because `Url::set_scheme` errors when changing to http/https let raw = url.to_string(); From f4c7ed199093629fd3bfdc37cd06ad8e94c83fb8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Aug 2024 10:35:16 -0500 Subject: [PATCH 4/4] fix(pkgid): Allow open namespaces in PackageIdSpec's --- .../src/core/package_id_spec.rs | 72 +++++++++++++++---- tests/testsuite/open_namespaces.rs | 14 +--- 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index c1ead804aaf..674d804ccb8 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -210,7 +210,10 @@ impl PackageIdSpec { } fn parse_spec(spec: &str) -> Result)>> { - let Some((name, ver)) = spec.split_once([':', '@']) else { + let Some((name, ver)) = spec + .rsplit_once('@') + .or_else(|| spec.rsplit_once(':').filter(|(n, _)| !n.ends_with(':'))) + else { return Ok(None); }; let name = name.to_owned(); @@ -438,7 +441,16 @@ mod tests { }, "foo", ); - err!("foo::bar", ErrorKind::PartialVersion(_)); + ok( + "foo::bar", + PackageIdSpec { + name: String::from("foo::bar"), + version: None, + url: None, + kind: None, + }, + "foo::bar", + ); ok( "foo:1.2.3", PackageIdSpec { @@ -449,7 +461,16 @@ mod tests { }, "foo@1.2.3", ); - err!("foo::bar:1.2.3", ErrorKind::PartialVersion(_)); + ok( + "foo::bar:1.2.3", + PackageIdSpec { + name: String::from("foo::bar"), + version: Some("1.2.3".parse().unwrap()), + url: None, + kind: None, + }, + "foo::bar@1.2.3", + ); ok( "foo@1.2.3", PackageIdSpec { @@ -460,7 +481,16 @@ mod tests { }, "foo@1.2.3", ); - err!("foo::bar@1.2.3", ErrorKind::PartialVersion(_)); + ok( + "foo::bar@1.2.3", + PackageIdSpec { + name: String::from("foo::bar"), + version: Some("1.2.3".parse().unwrap()), + url: None, + kind: None, + }, + "foo::bar@1.2.3", + ); ok( "foo@1.2", PackageIdSpec { @@ -635,9 +665,15 @@ mod tests { }, "path+file:///path/to/my/project/foo#bar", ); - err!( + ok( + "path+file:///path/to/my/project/foo#foo::bar", + PackageIdSpec { + name: String::from("foo::bar"), + version: None, + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, "path+file:///path/to/my/project/foo#foo::bar", - ErrorKind::PartialVersion(_) ); ok( "path+file:///path/to/my/project/foo#bar:1.1.8", @@ -649,9 +685,15 @@ mod tests { }, "path+file:///path/to/my/project/foo#bar@1.1.8", ); - err!( + ok( "path+file:///path/to/my/project/foo#foo::bar:1.1.8", - ErrorKind::PartialVersion(_) + PackageIdSpec { + name: String::from("foo::bar"), + version: Some("1.1.8".parse().unwrap()), + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, + "path+file:///path/to/my/project/foo#foo::bar@1.1.8", ); ok( "path+file:///path/to/my/project/foo#bar@1.1.8", @@ -663,9 +705,15 @@ mod tests { }, "path+file:///path/to/my/project/foo#bar@1.1.8", ); - err!( + ok( + "path+file:///path/to/my/project/foo#foo::bar@1.1.8", + PackageIdSpec { + name: String::from("foo::bar"), + version: Some("1.1.8".parse().unwrap()), + url: Some(Url::parse("file:///path/to/my/project/foo").unwrap()), + kind: Some(SourceKind::Path), + }, "path+file:///path/to/my/project/foo#foo::bar@1.1.8", - ErrorKind::PartialVersion(_) ); } @@ -676,8 +724,8 @@ mod tests { err!("baz@", ErrorKind::PartialVersion(_)); err!("baz@*", ErrorKind::PartialVersion(_)); err!("baz@^1.0", ErrorKind::PartialVersion(_)); - err!("https://baz:1.0", ErrorKind::PartialVersion(_)); - err!("https://#baz:1.0", ErrorKind::PartialVersion(_)); + err!("https://baz:1.0", ErrorKind::NameValidation(_)); + err!("https://#baz:1.0", ErrorKind::NameValidation(_)); err!( "foobar+https://github.com/rust-lang/crates.io-index", ErrorKind::UnsupportedProtocol(_) diff --git a/tests/testsuite/open_namespaces.rs b/tests/testsuite/open_namespaces.rs index c9b62579e77..4365af26604 100644 --- a/tests/testsuite/open_namespaces.rs +++ b/tests/testsuite/open_namespaces.rs @@ -379,15 +379,9 @@ fn update_spec_accepts_namespaced_name() { .run(); p.cargo("update foo::bar") .masquerade_as_nightly_cargo(&["open-namespaces"]) - .with_status(101) .with_stdout_data(str![""]) .with_stderr_data(str![[r#" -[ERROR] invalid package ID specification: `foo::bar` - - Did you mean `foo::bar`? - -Caused by: - expected a version like "1.32" +[LOCKING] 0 packages to latest compatible versions "#]]) .run() @@ -415,13 +409,9 @@ fn update_spec_accepts_namespaced_pkgid() { .run(); p.cargo(&format!("update path+{}#foo::bar@0.0.1", p.url())) .masquerade_as_nightly_cargo(&["open-namespaces"]) - .with_status(101) .with_stdout_data(str![""]) .with_stderr_data(str![[r#" -[ERROR] invalid package ID specification: `path+[ROOTURL]/foo#foo::bar@0.0.1` - -Caused by: - expected a version like "1.32" +[LOCKING] 0 packages to latest compatible versions "#]]) .run()