feat: add `cargo pkgid` support for cargo-script by 0xPoe · Pull Request #14961 · rust-lang/cargo (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits8 Checks21 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

What does this PR try to resolve?

close #14831

In this PR, we added the cargo pkgid support for the cargo-script.

For the package itself:

cargo pkgid --manifest-path foo.rs path+file:///my-project/foo.rs/foo#0.86.0

For its dependence:

cargo pkgid --manifest-path foo.rs -p sqlx registry+https://github.com/rust-lang/crates.io-index#sqlx@0.7.4

How should we test and review this PR?

I have updated the unit tests and also added more test cases for it.

Additional information

None

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Another test we'll need is that we error when a cargo script is specified as a dependency which might become possible with this change.

Another test we'll need is that we error when a cargo script is specified as a dependency which might become possible with this change.

Do you mean by cargo pkgid --manifest-path foo.rs -p foo?

I mean

[package] name = "foo"

[dependencies] bar.path = "bar.rs"

This comment has been minimized.

@epage dedc674 (#14961)

I just did a really rough commit to prove the idea here. It seems to work. But the code is totally messed up.

I want to check two things with you before I move forward and improve the code.

Can you take a grlance at the implementation, I think we have to change the SourceId and PackageIdSpec. I just want to check if I am on the right track.
I am a little hesitant that should we include the filename or just mark it is embedded. Since from the RFC, we don't allow users to specify the name at all, it seems the information is enough to construct the filename. But I also believe that including the full name would be easier to use and expand in the future. So I am not sure what is your thought.

I am a little hesitant that should we include the filename or just mark it is embedded. Since from the RFC, we don't allow users to specify the name at all, it seems the information is enough to construct the filename. B

The RFC says package.name is defaulted but users can set it.

package.name cannot be mapped unambiguously back to a file name because we sanitize it

let name = sanitize_name(file_stem.as_ref());

/// Ensure the package name matches the validation from `ops::cargo_new::check_name`
fn sanitize_name(name: &str) -> String {
let placeholder = if name.contains('_') {
'_'
} else {
// Since embedded manifests only support `[[bin]]`s, prefer arrow-case as that is the
// more common convention for CLIs
'-'
};

let mut name = PackageName::sanitize(name, placeholder).into_inner();

loop {
if restricted_names::is_keyword(&name) {
name.push(placeholder);
} else if restricted_names::is_conflicting_artifact_name(&name) {
// Being an embedded manifest, we always assume it is a `[[bin]]`
name.push(placeholder);
} else if name == "test" {
name.push(placeholder);
} else if restricted_names::is_windows_reserved(&name) {
// Go ahead and be consistent across platforms
name.push(placeholder);
} else {
break;
}
}

name
}

Can you take a grlance at the implementation, I think we have to change the SourceId and PackageIdSpec. I just want to check if I am on the right track.

// FIXME: It should be `path+[ROOTURL]/foo#script.rs@0.0.0`.
p.cargo("-Zscript pkgid --manifest-path script.rs")
    .masquerade_as_nightly_cargo(&["script"])
    .with_stdout_data(str![[r#"

path+[ROOTURL]/foo#script@0.0.0 "#]])

this should instead be

// FIXME: It should be `path+[ROOTURL]/foo/script.rs#0.0.0`.
p.cargo("-Zscript pkgid --manifest-path script.rs")
    .masquerade_as_nightly_cargo(&["script"])
    .with_stdout_data(str![[r#"

path+[ROOTURL]/foo/script.rs#0.0.0 "#]])

according to #14831 (comment)

Switching to that scheme should simplify the code, not requiring updates to PackageIdSpec, PackageId, or SourceId struct fields, only functions

The RFC says package.name is defaulted but users can set it.

Oh, I misunderstood package.name = <slugified file stem>. I thought it always used the file name.

package.name cannot be mapped unambiguously back to a file name because we sanitize it

Yes. Got it.

this should instead be

// FIXME: It should be path+[ROOTURL]/foo/script.rs#0.0.0. p.cargo("-Zscript pkgid --manifest-path script.rs") .masquerade_as_nightly_cargo(&["script"]) .with_stdout_data(str![[r#" path+[ROOTURL]/foo/script.rs#0.0.0 "#]])

according to #14831 (comment)

Switching to that scheme should simplify the code, not requiring updates to PackageIdSpec, PackageId, or SourceId struct fields, only functions

So I guess if the package name is foo, then we have:

path+[ROOTURL]/foo/script.rs#foo@0.0.0