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

Extend pkgid syntax with @ support #10582

Merged
merged 2 commits into from
May 6, 2022
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
8 changes: 4 additions & 4 deletions src/cargo/core/compiler/future_incompat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ fn render_report(per_package_reports: &[FutureIncompatReportPackage]) -> BTreeMa
let mut report: BTreeMap<String, String> = BTreeMap::new();
for per_package in per_package_reports {
let package_spec = format!(
"{}:{}",
"{}@{}",
per_package.package_id.name(),
per_package.package_id.version()
);
Comment on lines 237 to 242
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And with the following line:

let rendered = report.entry(package_spec).or_default();

This changes how the reports are created on disk. The report future-incompat --package flag doesn't take a package-spec but whatever was put into the report here.

  • We could just keep this as : but then report future-incompat will only accept :
  • We could have report future-incompat try to generate both formats

Any thoughts? @joshtriplett since I think you had worked on this

Copy link
Member

Choose a reason for hiding this comment

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

@epage We discussed this in the Cargo meeting; summary: go ahead and change it, don't worry about : at all. There's a format version number that you could bump so that old Cargo doesn't think it can read this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not dug further but from what I've seen of the code and failure modes, I don't think we even need to bump the format version number. It looks like report future-incompat is completely neutral to the pkgid spec format. It accepts a parameter and uses it as a key. You should be able to have old cargo's read the file.

Expand Down Expand Up @@ -415,10 +415,10 @@ You may want to consider updating them to a newer version to see if the issue ha
let manifest = bcx.packages.get_one(*package_id).unwrap().manifest();
format!(
"
- {name}
- {package_spec}
- Repository: {url}
- Detailed warning command: `cargo report future-incompatibilities --id {id} --package {name}`",
name = format!("{}:{}", package_id.name(), package_id.version()),
- Detailed warning command: `cargo report future-incompatibilities --id {id} --package {package_spec}`",
package_spec = format!("{}@{}", package_id.name(), package_id.version()),
url = manifest
.metadata()
.repository
Expand Down
45 changes: 38 additions & 7 deletions src/cargo/core/package_id_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ impl PackageIdSpec {
/// "https://crates.io/foo",
/// "https://crates.io/foo#1.2.3",
/// "https://crates.io/foo#bar:1.2.3",
/// "https://crates.io/foo#[email protected]",
/// "foo",
/// "foo:1.2.3",
/// "[email protected]",
/// ];
/// for spec in specs {
/// assert!(PackageIdSpec::parse(spec).is_ok());
Expand All @@ -65,7 +67,7 @@ impl PackageIdSpec {
);
}
}
let mut parts = spec.splitn(2, ':');
let mut parts = spec.splitn(2, [':', '@']);
let name = parts.next().unwrap();
let version = match parts.next() {
Some(version) => Some(version.to_semver()?),
Expand Down Expand Up @@ -122,7 +124,7 @@ impl PackageIdSpec {
})?;
match frag {
Some(fragment) => {
let mut parts = fragment.splitn(2, ':');
let mut parts = fragment.splitn(2, [':', '@']);
let name_or_version = parts.next().unwrap();
match parts.next() {
Some(part) => {
Expand Down Expand Up @@ -268,7 +270,7 @@ impl PackageIdSpec {
}
for id in ids {
if version_cnt[id.version()] == 1 {
msg.push_str(&format!("\n {}:{}", spec.name(), id.version()));
msg.push_str(&format!("\n {}@{}", spec.name(), id.version()));
} else {
msg.push_str(&format!("\n {}", PackageIdSpec::from_package_id(*id)));
}
Expand All @@ -290,11 +292,11 @@ impl fmt::Display for PackageIdSpec {
}
None => {
printed_name = true;
write!(f, "{}", self.name)?
write!(f, "{}", self.name)?;
}
}
if let Some(ref v) = self.version {
write!(f, "{}{}", if printed_name { ":" } else { "#" }, v)?;
write!(f, "{}{}", if printed_name { "@" } else { "#" }, v)?;
}
Ok(())
}
Expand Down Expand Up @@ -329,10 +331,11 @@ mod tests {

#[test]
fn good_parsing() {
fn ok(spec: &str, expected: PackageIdSpec) {
#[track_caller]
fn ok(spec: &str, expected: PackageIdSpec, expected_rendered: &str) {
let parsed = PackageIdSpec::parse(spec).unwrap();
assert_eq!(parsed, expected);
assert_eq!(parsed.to_string(), spec);
assert_eq!(parsed.to_string(), expected_rendered);
}

ok(
Expand All @@ -342,6 +345,7 @@ mod tests {
version: None,
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo",
);
ok(
"https://crates.io/foo#1.2.3",
Expand All @@ -350,6 +354,7 @@ mod tests {
version: Some("1.2.3".to_semver().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#1.2.3",
);
ok(
"https://crates.io/foo#bar:1.2.3",
Expand All @@ -358,6 +363,16 @@ mod tests {
version: Some("1.2.3".to_semver().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.3".to_semver().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
"https://crates.io/foo#[email protected]",
);
ok(
"foo",
Expand All @@ -366,6 +381,7 @@ mod tests {
version: None,
url: None,
},
"foo",
);
ok(
"foo:1.2.3",
Expand All @@ -374,6 +390,16 @@ mod tests {
version: Some("1.2.3".to_semver().unwrap()),
url: None,
},
"[email protected]",
);
ok(
"[email protected]",
PackageIdSpec {
name: InternedString::new("foo"),
version: Some("1.2.3".to_semver().unwrap()),
url: None,
},
"[email protected]",
);
}

Expand All @@ -382,6 +408,9 @@ mod tests {
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("https://baz:1.0").is_err());
assert!(PackageIdSpec::parse("https://#baz:1.0").is_err());
}
Expand All @@ -397,5 +426,7 @@ mod tests {
assert!(!PackageIdSpec::parse("foo").unwrap().matches(bar));
assert!(PackageIdSpec::parse("foo:1.2.3").unwrap().matches(foo));
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));
}
}
9 changes: 3 additions & 6 deletions src/cargo/ops/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -896,20 +896,17 @@ pub fn yank(
let (mut registry, _, _) =
registry(config, token, index.as_deref(), reg.as_deref(), true, true)?;

let package_spec = format!("{}@{}", name, version);
if undo {
config
.shell()
.status("Unyank", format!("{}:{}", name, version))?;
config.shell().status("Unyank", package_spec)?;
registry.unyank(&name, &version).with_context(|| {
format!(
"failed to undo a yank from the registry at {}",
registry.host()
)
})?;
} else {
config
.shell()
.status("Yank", format!("{}:{}", name, version))?;
config.shell().status("Yank", package_spec)?;
registry
.yank(&name, &version)
.with_context(|| format!("failed to yank from the registry at {}", registry.host()))?;
Expand Down
6 changes: 3 additions & 3 deletions src/doc/man/cargo-pkgid.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ following:
SPEC Structure | Example SPEC
---------------------------|--------------
_name_ | `bitflags`
_name_`:`_version_ | `bitflags:1.0.4`
_name_`@`_version_ | `bitflags@1.0.4`
_url_ | `https://github.com/rust-lang/cargo`
_url_`#`_version_ | `https://github.com/rust-lang/cargo#0.33.0`
_url_`#`_name_ | `https://github.com/rust-lang/crates.io-index#bitflags`
_url_`#`_name_`:`_version_ | `https://github.com/rust-lang/cargo#crates-io:0.21.0`
_url_`#`_name_`:`_version_ | `https://github.com/rust-lang/cargo#crates-io@0.21.0`

## OPTIONS

Expand Down Expand Up @@ -75,7 +75,7 @@ Get the package ID for the given package instead of the current package.

2. Retrieve package specification for version 1.0.0 of `foo`:

cargo pkgid foo:1.0.0
cargo pkgid foo@1.0.0

3. Retrieve package specification for `foo` from crates.io:

Expand Down
6 changes: 3 additions & 3 deletions src/doc/man/generated_txt/cargo-pkgid.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ DESCRIPTION
+-----------------+--------------------------------------------------+
| name | bitflags |
+-----------------+--------------------------------------------------+
| name:version | bitflags:1.0.4 |
| name@version | bitflags@1.0.4 |
+-----------------+--------------------------------------------------+
| url | https://github.com/rust-lang/cargo |
+-----------------+--------------------------------------------------+
Expand All @@ -36,7 +36,7 @@ DESCRIPTION
| | https://github.com/rust-lang/crates.io-index#bitflags |
+-----------------+--------------------------------------------------+
| | |
| url#name:version | https://github.com/rust-lang/cargo#crates-io:0.21.0 |
| url#name:version | https://github.com/rust-lang/cargo#crates-io@0.21.0 |
+-----------------+--------------------------------------------------+

OPTIONS
Expand Down Expand Up @@ -133,7 +133,7 @@ EXAMPLES

2. Retrieve package specification for version 1.0.0 of foo:

cargo pkgid foo:1.0.0
cargo pkgid foo@1.0.0

3. Retrieve package specification for foo from crates.io:

Expand Down
6 changes: 3 additions & 3 deletions src/doc/src/commands/cargo-pkgid.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ following:
SPEC Structure | Example SPEC
---------------------------|--------------
_name_ | `bitflags`
_name_`:`_version_ | `bitflags:1.0.4`
_name_`@`_version_ | `bitflags@1.0.4`
_url_ | `https://github.com/rust-lang/cargo`
_url_`#`_version_ | `https://github.com/rust-lang/cargo#0.33.0`
_url_`#`_name_ | `https://github.com/rust-lang/crates.io-index#bitflags`
_url_`#`_name_`:`_version_ | `https://github.com/rust-lang/cargo#crates-io:0.21.0`
_url_`#`_name_`:`_version_ | `https://github.com/rust-lang/cargo#crates-io@0.21.0`

## OPTIONS

Expand Down Expand Up @@ -159,7 +159,7 @@ details on environment variables that Cargo reads.

2. Retrieve package specification for version 1.0.0 of `foo`:

cargo pkgid foo:1.0.0
cargo pkgid foo@1.0.0

3. Retrieve package specification for `foo` from crates.io:

Expand Down
14 changes: 7 additions & 7 deletions src/doc/src/reference/pkgid-spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ is a string which is used to uniquely refer to one package within a graph of
packages.

The specification may be fully qualified, such as
`https://github.com/rust-lang/crates.io-index#regex:1.4.3` or it may be
`https://github.com/rust-lang/crates.io-index#regex@1.4.3` or it may be
abbreviated, such as `regex`. The abbreviated form may be used as long as it
uniquely identifies a single package in the dependency graph. If there is
ambiguity, additional qualifiers can be added to make it unique. For example,
if there are two versions of the `regex` package in the graph, then it can be
qualified with a version to make it unique, such as `regex:1.4.3`.
qualified with a version to make it unique, such as `regex@1.4.3`.

#### Specification grammar

Expand All @@ -23,7 +23,7 @@ The formal grammar for a Package Id Specification is:
```notrust
spec := pkgname
| proto "://" hostname-and-path [ "#" ( pkgname | semver ) ]
pkgname := name [ ":" semver ]
pkgname := name [ ("@" | ":" ) semver ]
proto := "http" | "git" | ...
```
Expand All @@ -40,17 +40,17 @@ The following are references to the `regex` package on `crates.io`:
| Spec | Name | Version |
|:------------------------------------------------------------|:-------:|:-------:|
| `regex` | `regex` | `*` |
| `regex:1.4.3` | `regex` | `1.4.3` |
| `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` |
| `https://github.com/rust-lang/crates.io-index#regex@1.4.3` | `regex` | `1.4.3` |

The following are some examples of specs for several different git dependencies:

| Spec | Name | Version |
|:----------------------------------------------------------|:----------------:|:--------:|
| `https://github.com/rust-lang/cargo#0.52.0` | `cargo` | `0.52.0` |
| `https://github.com/rust-lang/cargo#cargo-platform:0.1.2` | <nobr>`cargo-platform`</nobr> | `0.1.2` |
| `ssh://[email protected]/rust-lang/regex.git#regex:1.4.3` | `regex` | `1.4.3` |
| `https://github.com/rust-lang/cargo#cargo-platform@0.1.2` | <nobr>`cargo-platform`</nobr> | `0.1.2` |
| `ssh://[email protected]/rust-lang/regex.git#regex@1.4.3` | `regex` | `1.4.3` |

Local packages on the filesystem can use `file://` URLs to reference them:

Expand Down
8 changes: 4 additions & 4 deletions src/etc/man/cargo-pkgid.1
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ T}:T{
\fBbitflags\fR
T}
T{
\fIname\fR\fB:\fR\fIversion\fR
\fIname\fR\fB@\fR\fIversion\fR
T}:T{
\fBbitflags:1.0.4\fR
\fBbitflags@1.0.4\fR
T}
T{
\fIurl\fR
Expand All @@ -58,7 +58,7 @@ T}
T{
\fIurl\fR\fB#\fR\fIname\fR\fB:\fR\fIversion\fR
T}:T{
\fBhttps://github.com/rust\-lang/cargo#crates\-io:0.21.0\fR
\fBhttps://github.com/rust\-lang/cargo#crates\-io@0.21.0\fR
T}
.TE
.sp
Expand Down Expand Up @@ -195,7 +195,7 @@ cargo pkgid foo
.sp
.RS 4
.nf
cargo pkgid foo:1.0.0
cargo pkgid foo@1.0.0
.fi
.RE
.RE
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/credential_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ fn yank() {
.with_stderr(
"\
[UPDATING] [..]
[YANK] foo:0.1.0
[YANK] foo@0.1.0
",
)
.run();
Expand Down
10 changes: 5 additions & 5 deletions tests/testsuite/future_incompat_report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ frequency = 'never'
.env("RUSTFLAGS", "-Zfuture-incompat-test")
.with_stderr_contains(FUTURE_OUTPUT)
.with_stderr_contains("warning: the following packages contain code that will be rejected by a future version of Rust: foo v0.0.0 [..]")
.with_stderr_contains(" - foo:0.0.0[..]")
.with_stderr_contains(" - foo@0.0.0[..]")
.run();
}
}
Expand Down Expand Up @@ -189,17 +189,17 @@ fn test_multi_crate() {
p.cargo(command).arg("--future-incompat-report")
.env("RUSTFLAGS", "-Zfuture-incompat-test")
.with_stderr_contains("warning: the following packages contain code that will be rejected by a future version of Rust: first-dep v0.0.1, second-dep v0.0.2")
.with_stderr_contains(" - first-dep:0.0.1")
.with_stderr_contains(" - second-dep:0.0.2")
.with_stderr_contains(" - first-dep@0.0.1")
.with_stderr_contains(" - second-dep@0.0.2")
.run();

p.cargo("report future-incompatibilities").arg("--package").arg("first-dep:0.0.1")
p.cargo("report future-incompatibilities").arg("--package").arg("first-dep@0.0.1")
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 test failed with : until I updated it because the package id is being written to the report.

.with_stdout_contains("The package `first-dep v0.0.1` currently triggers the following future incompatibility lints:")
.with_stdout_contains(FUTURE_OUTPUT)
.with_stdout_does_not_contain("[..]second-dep-0.0.2/src[..]")
.run();

p.cargo("report future-incompatibilities").arg("--package").arg("second-dep:0.0.2")
p.cargo("report future-incompatibilities").arg("--package").arg("second-dep@0.0.2")
.with_stdout_contains("The package `second-dep v0.0.2` currently triggers the following future incompatibility lints:")
.with_stdout_contains(FUTURE_OUTPUT)
.with_stdout_does_not_contain("[..]first-dep-0.0.1/src[..]")
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1750,8 +1750,8 @@ fn update_ambiguous() {
is ambiguous.
Please re-run this command with `-p <spec>` where `<spec>` is one of the \
following:
bar:0.[..].0
bar:0.[..].0
bar@0.[..].0
bar@0.[..].0
",
)
.run();
Expand Down
Loading