feat: Initial support for single-file packages #12245

epage · 2023-06-09T01:42:14Z

What does this PR try to resolve?

This is the first step towards #12207. In particular, this focuses on pulling in the demo 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 news 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 :)

rustbot · 2023-06-09T01:42:19Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

This should be more resilient to false positives like in rust-lang#12245 where a string contains `println`.

chore: Migrate print-ban from test to clippy This should be more resilient to false positives like in #12245 where a string contains `println`. See also #12224

I decided to start things off fairly simple. It either looks like the user is specifying a manifest or they aren't. This is assuming we only need to handle `cargo foo.rs` and `cargo ./foo.rs` and not `cargo foo`. I think in most shebang cases, path multiple components will present which makes that a dead giveaway and likely to not overlap with aliases and subcommands. For reference, dlang's dub goes a lot further 1. Checks for the subcommand name being `-` 2. Checks if subcommand name ends with `.d` 3. Checks if subcommand name is built-in 4. Checks if a file with the subcommand name exists 5. Checks if a file with the subcommand name + `.d` exists This would allow a `run.d` to override `dub-run` which doesn't seem good.

epage · 2023-06-09T19:38:42Z

FYI @weihanglo I've finally appeased the CI gods

weihanglo

Just did the first round review, yet I skipped the juicy part — toml/embedded.rs. Thanks for making this happen. I'll get back in the next few days!

weihanglo · 2023-06-09T08:59:49Z

Cargo.toml

@@ -13,6 +13,7 @@ exclude = [
 [workspace.dependencies]
 anyhow = "1.0.47"
 base64 = "0.21.0"
+blake3 = "1.3.3"


Could you clarify that why we need blake3? Is it only for performance?

I was just looking for what a recommended hashing algorithm is. This is used for creating the temporary manifest on disk and will be going away when we get native support for embedded manifests. Similarly, we'll be dropping the regex dependency when we change up the parsing of embedded manifests.

Not really a blocker but maybe something like this at this moment would better.

diff --git a/src/cargo/util/toml/embedded.rs b/src/cargo/util/toml/embedded.rs index 875ec6a4b..833f90ce9 100644 --- a/src/cargo/util/toml/embedded.rs +++ b/src/cargo/util/toml/embedded.rs @@ -68,7 +68,7 @@ impl RawScript { config: &Config, target_dir: &std::path::Path, ) -> CargoResult<std::path::PathBuf> { - let hash = self.hash().to_string(); + let hash = self.hash(); assert_eq!(hash.len(), 64); let mut workspace_root = target_dir.to_owned(); workspace_root.push("eval"); @@ -207,8 +207,8 @@ impl RawScript { Ok(slug) } - fn hash(&self) -> blake3::Hash { - blake3::hash(self.body.as_bytes()) + fn hash(&self) -> String { + cargo_util::Sha256::new().update(self.body.as_bytes()).finish_hex() } }

Can we defer this out? Scott already has a branch where the hashing can be dropped, so dropping it should be a quick follow on. My hope with this change was to have cargo-script-mvs as a baseline and would rather not have churn related to throwaway parts of the PR

A tiny drawback of introducing new dependendies: need to add license exceptions on Rust side rust-lang/rust#112601.

This is not this PR's fault. It is the license check is out of sync between rust-lang/rust and rust-lang/cargo.

weihanglo · 2023-06-09T23:12:51Z

src/bin/cargo/commands/run.rs

@@ -101,3 +105,16 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
        }
    })
 }
+
+pub fn is_manifest_command(arg: &str) -> bool {


(not a blocker)
“Manifest commands” is already a term, though its definition is not pretty clear to me.

https://doc.rust-lang.org/nightly/cargo/commands/manifest-commands.html

crates/cargo-test-support/src/lib.rs

src/bin/cargo/cli.rs

tests/testsuite/script.rs

deny.toml

src/bin/cargo/cli.rs

This will give us a window to collect feedback on if this affects anyone.

This is no where near the implementation we want but I think we should develop incrementally on top of what we already have. See https://github.com/epage/cargo-script-mvs/tree/main

This is written to reflect the current implementation though some parts might read a little weird because I didn't want to write throw-away documentation for when we change this. For example, single-file packages are currently only supported in `cargo <command>` and not as manifest paths but this will change.

weihanglo

Given this is an implementation for an eRFC, and it has only little impact on the current behavior of Cargo, I am pretty open to merge it as-is.

@epage I left some comment for potential cleanup but feel free to r=weihanglo if they don't make sense to do at this moment. I think what to should do is writing down the decisions needed to be made and move on.

cargo.before

src/bin/cargo/cli.rs

weihanglo · 2023-06-12T13:09:34Z