Tracking Issue for cargo-script RFC 3424 #12207

ehuss · 2023-05-31T03:55:52Z

feat: Initial support for single-file packages ### What does this PR try to resolve? This is the first step towards #12207. In particular, this focuses on pulling in the [demo](https://github.com/epage/cargo-script-mvs) roughly as-is to serve as a baseline for further PRs. I have a couple months of runtime (multiple times a week) using the version of the demo included here. ### How should we test and review this PR? Commit-by-commit. Most likely, the last (docs) commit should be done first to provide context for the others. Naming is hard. I came up with these terms just so we can have ways to refer to them. Feedback is welcome. - `-Zscript` for this general feature (not great but didn't want to spend too long coming up with a throwaway name) - "single-file package": Rust code and a cargo manifest in a single file - "embedded manifest": the explicit manifest inside of a single-file package - "manifest command": when we interpret `cargo <name>` as referring to a single-file package, and similar to "built-in commands" and "external commands". Keep in mind that this is a very hacky solution with many deficiencies and is mostly starting as a baseline for implementing and reviewing those improvements, including - Switching from `regex` to `syn` for extracting manifests for greater resilience - Matching `cargo new`s logic when sanitizing the inferred package name - Allowing `cargo <name>` to also be a `Cargo.toml` file (for consistency in where manifests are accepted) - Allowing single-file packages to be used wherever a manifest is accepted To minimize conflict pain, I would ask that we consider what feedback can be handled in a follow up (though still mention it!). It'll be much easier creating multiple, independent PRs once this baseline is merged to address concerns. ### Additional information The only affect for people on stable is that they may get a warning if they have an external subcommand that will be shadowed when this feature is implemented. This will allow us to collect feedback, without blocking people, so we can have an idea of how "safe" our precedence scheme is for interpreting `cargo <name>`. As of right now, aliases with a `.` in them will be ignored (well, technically their suffix will be exposed as an alias). We directly inject the name into a lookup string into the config that uses `.` as the separator, so we drill down until we get to the leaf element. Ideally, we would be generating / parsing the lookup key using TOML key syntax so we can better report that this won't be supported after this change :)

fix(embedded): Align package name sanitization with cargo-new ### What does this PR try to resolve? This is a follow up to #12245 which is working to resolve the tracking issue #12207 This first aligns sanitization of package names with the central package name validation logic, putting the code next to each other so they can more easily stay in sync. Oddly enough, cargo-new is stricter than our normal package-name validation. I went ahead and sanitized along with that as well. In working on this, I was bothered by - the mix of `-` and `_` in file names because of sanitization, so I made it more consistent by detecting which the user is using - -using `_` in bins, so I switched the default to `-` ### How should we test and review this PR? One existing test covers a variety of sanitization needs Another existing test hit one of the other cases (`test` being reserved) ### Additional information For implementation convenience, I changed the directory we write the manifest to. The impact of this should be minimal since - We reuse the full file name, so if it worked for the user it should work for us - We should be moving away from the temp manifest in future commits

refactor(embedded): Switch to `syn` for parsing doc comments This is a follow up to #12245 which is working to resolve #12207 The hope is this will result in more resilient comment handling, being more consistent with rustdoc. I also hoped for less code but `syn` is doing less than I had expected, requiring us to copy code over from other parts of rust. It seems every proc macro has to do this but there is no guide to it, so they all do it differently, only covering the cases they thought to test for. Note that this still won't support `include_str!()`.

fix(embeded): Don't pollute the scripts dir with `target/` ### What does this PR try to resolve? This PR is part of #12207. This specific behavior was broken in #12268 when we stopped using an intermediate `Cargo.toml` file. Unlike pre-#12268, - We are hashing the path, rather than the content, with the assumption that people change content more frequently than the path - We are using a simpler hash than `blake3` in the hopes that we can get away with it Unlike the Pre-RFC demo - We are not forcing a single target dir for all scripts in the hopes that we get #5931 ### How should we test and review this PR? A new test was added specifically to show the target dir behavior, rather than overloading an existing test or making all tests sensitive to changes in this behavior. ### Additional information In the future, we might want to resolve symlinks before we get to this point

Rustin170506 · 2024-09-18T15:27:56Z

Hi @epage, as a daily user of cargo-script, I really want to help advance and stabilize this feature. (Thank you all for working on this feature.) What can I do to help? Is there anything you would suggest for me to get started?

epage · 2024-09-18T15:41:30Z

@Rustin170506 any of the tasks in the task list without a PR is good for grabbing.

Some clarifications

Resolve symlinks in cargo ?

The use case for this is that we allow cargo Cargo.toml but putting a #! in Cargo.toml only works well if you symlink it into path with the name of the bin.

I had wondered if we should do just always resolve symlinks but that breaks some use cases. Instead, when determining whether its embedded or not and there is no extension, we should resolve it to see what the

Change config paths to only check CARGO_HOME

cargo/src/bin/cargo/commands/run.rs

Lines 176 to 181 in 0461165

    
           // Treat `cargo foo.rs` like `cargo install --path foo` and re-evaluate the config based on the 
        
           // location where the script resides, rather than the environment from where it's being run. 
        
           let parent_path = manifest_path 
        
               .parent() 
        
               .expect("a file should always have a parent"); 
        
           gctx.reload_rooted_at(parent_path)?;

When we talked about this as a Cargo team, we decided this need to reload to cargo home, like cargo install does, to avoid downloading a script and being subject to spurious files in your ~/Downloads, at the cost of losing the config from your repo.

#14476

This might require an RFC but likely a trivial one

cargo pkgid support

This will be messy. We'll likely need to change SourceId to include the file name if it has an embedded manifest.

Skgland · 2024-09-18T15:53:53Z

One thing that occurred to me while just reading this is as you mention downloaded files :

Should cargo check for a Mark of the Web when running/building scripts on Windows/NTFS-drives?

pirafrank · 2024-09-18T16:08:09Z

One thing that occurred to me while just reading this is as you mention downloaded files :

Should cargo check for a Mark of the Web when running/building scripts on Wwindows/NTFS-drives?

I don't think so, but if it does, it should do it on macOS too, by checking additional file attributes.

feat: add CARGO_MANIFEST_PATH env variable Adds `CARGO_MANIFEST_PATH` variable as part of #12207 Context: `CARGO_MANIFEST_DIR` is not very useful, because there is no `Cargo.toml` file when running a cargo script. In cases when multiple scripts are stored in the same folder, we can't tell which script exactly is being run using `CARGO_MANIFEST_DIR`

Rustin170506 · 2024-10-31T15:01:13Z

This will be messy. We'll likely need to change SourceId to include the file name if it has an embedded manifest.

I will give this one a try.

This avoids syntax errors until Rust supports the syntax in its tooling: rust-lang/cargo#12207 (comment)

Rustin170506 · 2024-11-11T14:20:58Z

This will be messy. We'll likely need to change SourceId to include the file name if it has an embedded manifest.

Not sure if this is the right place to discuss implementation details, but I wanted to ask if the output makes sense.

$pwd
/Users/rustin/code/rs-scripts

$ls
sql.rs  sql1.rs sql2.rs sql3.rs sql4.rs sql5.rs sql6.rs sql7.rs sql8.rs sql9.rs

$cargo pkgid sql2.rs
path+file:///Users/rustin/code/rs-scripts/sql2.rs#embedded

epage · 2024-11-11T14:40:55Z

@Rustin170506 I feel like there might be enough discussion on that that maybe we should create a dedicated issue?

### What does this PR try to resolve? We removed rejected syntax in #13861 haven't update frontmatter parsing to what [RFC 3503](https://rust-lang.github.io/rfcs/3503-frontmatter.html#reference-level-explanation) says. This adds tests and fixes various cases to ensure we correctly detect the frontmatter and the infostring. This is part of #12207 ### How should we test and review this PR? ### Additional information

### What does this PR try to resolve? This adds support for cargo script for more cargo commands and is a part of #12207 Unfortunately, there is a double-warning for unspecified editions for `cargo remove`. Rather than addressing that here, I'll be noting it in the tracking issue. ### How should we test and review this PR? ### Additional information

At this point it is unlikely for a script to be written for pre-2024 edition and migrated to 2024+ but this code is independent of Editions so this means we have one less bug and are better prepared for the next edition. When we add `cargo fix` support for regular manifest warnings, we'll need to take into account cargo scripts. This is a part of rust-lang#12207.

### What does this PR try to resolve? At this point it is unlikely for a script to be written for pre-2024 edition and migrated to 2024+ but this code is independent of Editions so this means we have one less bug and are better prepared for the next edition. When we add `cargo fix` support for regular manifest warnings, we'll need to take into account cargo scripts. This is a part of #12207. ### How should we test and review this PR? ### Additional information While looking for where the tests go, I found a couple places with a hard coded latest-edition in test output and fixed it.

This has been around since rust-lang#12224 and isn't in the RFC, so its being removed. This is part of rust-lang#12207.

This has been around since #12224 and isn't in the RFC, so its being removed. This is part of #12207.

ehuss added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review C-tracking-issue Category: A tracking issue for something unstable. Z-script Nightly: cargo script labels May 31, 2023

ehuss mentioned this issue May 31, 2023

eRFC: single-file packages ("cargo script") integration rust-lang/rfcs#3424

Merged