feat: add `cargo pkgid` support for cargo-script #14961

Rustin170506 · 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.

Rustin170506 · 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"

Rustin170506 · 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

Rustin170506 · 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.

Rustin170506 · 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

src/cargo/core/workspace.rs

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

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

tests/testsuite/update.rs

tests/testsuite/script.rs

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

Rustin170506

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

Rustin170506 · 2025-01-15T15:36:36Z

src/cargo/sources/path.rs

@@ -124,6 +124,9 @@ impl<'gctx> PathSource<'gctx> {
    }

    fn read_package(&self) -> CargoResult<Package> {
+        if crate::util::toml::is_embedded(&self.path) {


I am not sure if this is the best place to check this. Maybe we should move it to where we create the source ID?

imo this feels too spooky as this function is not directly tied to dependencies.

Then I guess we should do the check when we read the manifest file.

Or the dependency resolution phase. Maybe do it when resolving dependencies makes more sense.

epage · 2025-01-15T15:49:33Z

src/cargo/core/workspace.rs

+        ws.packages
+            .packages
+            .insert(ws.current_manifest.clone(), package);
        ws.target_dir = if let Some(dir) = target_dir {


Does this reformatting belong on the previous commit?

Oops. I squashed it into the wrong commit.

epage · 2025-01-15T15:50:38Z

tests/testsuite/script.rs

@@ -1370,6 +1370,47 @@ registry+https://github.com/rust-lang/crates.io-index#[email protected]
        .run();
 }

+#[cargo_test(nightly, reason = "edition2024 hasn't hit stable yet")]
+fn script_as_dep() {


Is there a reason this test wasn't added before the feature commit?

Just want to make sure it already has an error for it. The feature is just to improve the error message.

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

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

Rustin170506 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

Rustin170506 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

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved

Rustin170506 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]>

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

Rustin170506 changed the title ~~NOT READY FOR REVIEW : feat: add cargo pkgid support for cargo-script~~ feat: add cargo pkgid support for cargo-script Jan 2, 2025

Rustin170506 force-pushed the rustin-patch-pkgid branch 3 times, most recently from 749889c to 82cbacd Compare January 9, 2025 15:20

Rustin170506 marked this pull request as ready for review January 9, 2025 17:04

Rustin170506 marked this pull request as draft January 10, 2025 15:28

Rustin170506 force-pushed the rustin-patch-pkgid branch 2 times, most recently from e98e69a to 6982520 Compare January 11, 2025 15:26

Rustin170506 commented Jan 11, 2025

View reviewed changes

tests/testsuite/update.rs Outdated Show resolved Hide resolved

Rustin170506 commented Jan 11, 2025

View reviewed changes

tests/testsuite/script.rs Show resolved Hide resolved

Rustin170506 force-pushed the rustin-patch-pkgid branch 2 times, most recently from 497943e to b62a3bc Compare January 11, 2025 16:31

Rustin170506 added 4 commits January 14, 2025 00:02

refactor: use manifest path as the key

eb043fc

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

feat: use full URL for embedded script paths

7fcac4e

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

test: add test for script as a dependency

d8be32e

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

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

a4ca61d

Rustin170506 force-pushed the rustin-patch-pkgid branch from bff4962 to a4ca61d Compare January 13, 2025 16:02

Rustin170506 commented Jan 15, 2025

View reviewed changes

Rustin170506 marked this pull request as ready for review January 15, 2025 15:36

Rustin170506 requested a review from epage January 15, 2025 15:36

epage reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `cargo pkgid` support for cargo-script #14961

feat: add `cargo pkgid` support for cargo-script #14961

Rustin170506 commented Dec 19, 2024 •

edited

Loading

rustbot commented Dec 19, 2024

epage commented Dec 19, 2024

Rustin170506 commented Dec 19, 2024 •

edited

Loading

epage commented Dec 19, 2024

This comment has been minimized.

Rustin170506 commented Dec 31, 2024

epage commented Dec 31, 2024

epage commented Dec 31, 2024

Rustin170506 commented Jan 2, 2025

Rustin170506 commented Jan 2, 2025 •

edited

Loading

epage commented Jan 2, 2025

Rustin170506 left a comment

Rustin170506 Jan 15, 2025

epage Jan 15, 2025

Rustin170506 Jan 15, 2025

Rustin170506 Jan 15, 2025

epage Jan 15, 2025

Rustin170506 Jan 15, 2025

epage Jan 15, 2025

Rustin170506 Jan 15, 2025

feat: add cargo pkgid support for cargo-script #14961

Are you sure you want to change the base?

feat: add cargo pkgid support for cargo-script #14961

Conversation

Rustin170506 commented Dec 19, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 19, 2024

epage commented Dec 19, 2024

Rustin170506 commented Dec 19, 2024 • edited Loading

epage commented Dec 19, 2024

This comment has been minimized.

Rustin170506 commented Dec 31, 2024

epage commented Dec 31, 2024

epage commented Dec 31, 2024

Rustin170506 commented Jan 2, 2025

Rustin170506 commented Jan 2, 2025 • edited Loading

epage commented Jan 2, 2025

Rustin170506 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `cargo pkgid` support for cargo-script #14961

feat: add `cargo pkgid` support for cargo-script #14961

Rustin170506 commented Dec 19, 2024 •

edited

Loading

Rustin170506 commented Dec 19, 2024 •

edited

Loading

Rustin170506 commented Jan 2, 2025 •

edited

Loading