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

0xPoe · 2024-12-19T14:31:09Z

What does this PR try to resolve?

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#[email protected]

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

rustbot · 2024-12-19T14:31:15Z

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

epage · 2024-12-19T14:58:38Z

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.

0xPoe · 2024-12-19T15:19:37Z

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?

epage · 2024-12-19T15:37:41Z

I mean

[package]
name = "foo"

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

0xPoe · 2024-12-31T14:42:04Z

@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.

epage · 2024-12-31T15:57:38Z

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

cargo/src/cargo/util/toml/embedded.rs

Line 115 in 0499e31

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

cargo/src/cargo/util/toml/embedded.rs

Lines 147 to 176 in 0499e31

    
           /// 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 
        
           }

epage · 2024-12-31T16:01:26Z

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#[email protected]`.
    p.cargo("-Zscript pkgid --manifest-path script.rs")
        .masquerade_as_nightly_cargo(&["script"])
        .with_stdout_data(str![[r#"
path+[ROOTURL]/foo#[email protected]
"#]])

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

0xPoe · 2025-01-02T14:06:50Z

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.

0xPoe · 2025-01-02T14:13:02Z

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

epage · 2025-01-02T14:21:38Z

So I guess if the package name is foo, then we have:
path+[ROOTURL]/foo/script.rs#foo@0.0.0

If the user explicitly overrode it, yes

…ommand Signed-off-by: Rustin170506 <[email protected]>

Signed-off-by: Rustin170506 <[email protected]>

Signed-off-by: Rustin170506 <[email protected]> fix

Signed-off-by: Rustin170506 <[email protected]>

0xPoe

🔢 Self-check (PR reviewed by myself and ready for feedback.)

Signed-off-by: Rustin170506 <[email protected]>

epage · 2025-02-04T19:52:33Z

Thanks!

0xPoe · 2025-02-05T02:16:55Z

Thanks for your review! 💚 💙 💜 💛 ❤️

Update cargo 14 commits in 0e3d73849ab8cbbab3ec5c65cbd555586cb21339..2928e32734b04925ee51e1ae88bea9a83d2fd451 2025-02-01 20:14:40 +0000 to 2025-02-07 16:50:22 +0000 - Simplify backtrack (rust-lang/cargo#15150) - Don't use on Solaris libc::LOCK_* which were removed from libc in ver… (rust-lang/cargo#15143) - feat: emit error if package not found within workspace (rust-lang/cargo#15071) - Make cache tracking resilient to unexpected files (rust-lang/cargo#15147) - Small resolver cleanups (rust-lang/cargo#15040) - feat: add `cargo pkgid` support for cargo-script (rust-lang/cargo#14961) - Suggest similar feature names on CLI (rust-lang/cargo#15133) - fix: Don't use "did you mean" in errors (rust-lang/cargo#15138) - Fix changelog link (rust-lang/cargo#15142) - chore(deps): update rust crate rand to 0.9.0 (rust-lang/cargo#15129) - Remove the original changelog (rust-lang/cargo#15123) - chore(deps): update rust crate gix to 0.70.0 (rust-lang/cargo#15128) - allow windows reserved names in CI (rust-lang/cargo#15135) - removed a word that was repeated (rust-lang/cargo#15136)

This might have also broken dependencies whose path ends with `.rs` as well This broke in rust-lang#14961

…5240) ### What does this PR try to resolve? This likely only affects `-Zpackage-workspace` but it might have also broken dependencies whose path ends with `.rs` as well This broke in #14961 ### How should we test and review this PR? ### Additional information

…st-lang#15240) ### What does this PR try to resolve? This likely only affects `-Zpackage-workspace` but it might have also broken dependencies whose path ends with `.rs` as well This broke in rust-lang#14961 ### How should we test and review this PR? ### Additional information

rustbot assigned epage Dec 19, 2024

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-pkgid S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2024

0xPoe force-pushed the rustin-patch-pkgid branch from 73e3975 to 3e3383a Compare December 19, 2024 14:50

0xPoe force-pushed the rustin-patch-pkgid branch 2 times, most recently from 3022d26 to 44ded93 Compare December 19, 2024 15:12

rustbot added A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces Command-install Command-uninstall labels Dec 31, 2024

This comment has been minimized.

Sign in to view

0xPoe force-pushed the rustin-patch-pkgid branch 2 times, most recently from c10754e to 6659d5f Compare January 2, 2025 15:16

epage reviewed Jan 2, 2025

View reviewed changes

Comment thread src/cargo/core/workspace.rs Outdated

0xPoe added 3 commits January 2, 2025 23:17

fix: remove unsupported embedded workspace check from cargo pkgid c…

5465e6a

…ommand Signed-off-by: Rustin170506 <[email protected]>

test: add the no lock file case

feda97d

Signed-off-by: Rustin170506 <[email protected]>

test: add the dep case

0c2bb4e

Signed-off-by: Rustin170506 <[email protected]>

0xPoe force-pushed the rustin-patch-pkgid branch 2 times, most recently from d10f797 to fc25d9d Compare January 2, 2025 15:18

epage reviewed Jan 15, 2025

View reviewed changes

Comment thread src/cargo/core/workspace.rs

epage reviewed Jan 15, 2025

View reviewed changes

Comment thread tests/testsuite/script.rs

0xPoe added 3 commits January 20, 2025 21:53

refactor: use manifest path as the key

d4c1eee

Signed-off-by: Rustin170506 <[email protected]> fix

feat: use full URL for embedded script paths

95f6d0b

Signed-off-by: Rustin170506 <[email protected]>

test: add test for script as a dependency

1514c7e

Signed-off-by: Rustin170506 <[email protected]>

0xPoe force-pushed the rustin-patch-pkgid branch 2 times, most recently from 183eeff to 1d960c7 Compare January 25, 2025 14:55

0xPoe commented Jan 25, 2025

View reviewed changes

0xPoe requested a review from epage January 25, 2025 15:09

epage reviewed Jan 27, 2025

View reviewed changes

Comment thread src/cargo/core/source_id.rs Outdated

epage reviewed Jan 27, 2025

View reviewed changes

Comment thread src/cargo/core/source_id.rs

0xPoe added 2 commits February 4, 2025 22:57

fix: better error message for using the script as a dep

91da8db

Signed-off-by: Rustin170506 <[email protected]>

test: add a test for error handling in script installation

9fe45b2

Signed-off-by: Rustin170506 <[email protected]>

0xPoe force-pushed the rustin-patch-pkgid branch from b8aac6b to 9fe45b2 Compare February 4, 2025 14:57

0xPoe requested a review from epage February 4, 2025 16:01

epage approved these changes Feb 4, 2025

View reviewed changes

epage added this pull request to the merge queue Feb 4, 2025

epage mentioned this pull request Dec 11, 2024

Tracking Issue for cargo-script RFC 3424 #12207

Open

39 tasks

Merged via the queue into rust-lang:master with commit f651497 Feb 4, 2025

0xPoe deleted the rustin-patch-pkgid branch February 5, 2025 02:16

weihanglo mentioned this pull request Feb 8, 2025

Update cargo rust-lang/rust#136725

Merged

rustbot added this to the 1.86.0 milestone Feb 8, 2025

epage added a commit to epage/cargo that referenced this pull request Feb 27, 2025

fix(package): Ensure we can package directories ending with '.rs'

730348a

This might have also broken dependencies whose path ends with `.rs` as well This broke in rust-lang#14961

epage mentioned this pull request Feb 27, 2025

fix(package): Ensure we can package directories ending with '.rs' #15240

Merged

Conversation

0xPoe commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Dec 19, 2024

Uh oh!

epage commented Dec 19, 2024

Uh oh!

0xPoe commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Dec 19, 2024

Uh oh!

This comment has been minimized.

0xPoe commented Dec 31, 2024

Uh oh!

epage commented Dec 31, 2024

Uh oh!

epage commented Dec 31, 2024

Uh oh!

0xPoe commented Jan 2, 2025

Uh oh!

0xPoe commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Jan 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

epage commented Feb 4, 2025

Uh oh!

0xPoe commented Feb 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0xPoe commented Dec 19, 2024 •

edited

Loading

0xPoe commented Dec 19, 2024 •

edited

Loading

0xPoe commented Jan 2, 2025 •

edited

Loading