Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: cargo-script #3502

Merged
merged 59 commits into from
May 17, 2024
Merged

RFC: cargo-script #3502

merged 59 commits into from
May 17, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 26, 2023

Rendered
FCP

eRFC #3424 was previously approved without any direction on syntax or behavior having been decided. This RFC is to finalize the decisions made through the implementation to prepare the way for stabilization as we work through the remaining tasks in rust-lang/cargo#12207

#3503 covers the T-lang side of this conversation

Example:

#!/usr/bin/env cargo
---
[dependencies]
clap = { version = "4.2", features = ["derive"] }
---

use clap::Parser;

#[derive(Parser, Debug)]
#[clap(version)]
struct Args {
    #[clap(short, long, help = "Path to config")]
    config: Option<std::path::PathBuf>,
}

fn main() {
    let args = Args::parse();
    println!("{:?}", args);
}
$ ./prog --config file.toml
Args { config: Some("file.toml") }

Note: most of this is available behind -Zscript (see rust-lang/cargo#12207 for limitations). Support for the code-fence frontmatter was added in rust-lang/cargo#12681 and it may be a couple of days before cargo's submodule in rust-lang/rust gets updated and built into a nightly.

@epage epage added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Sep 26, 2023
@ggggggggg

This comment was marked as resolved.

This RFC adds support for single-file bin packages in cargo.
Single-file bin packages are `.rs` files with an embedded manifest and a
`main`.
These will be accepted with just like `Cargo.toml` files with
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggggggggg

"These will be accepted with just like Cargo.toml files" doesn't read right.

I've moved this thread to the context to make it easier to follow.

Could you clarify why you feel this isn't right?

Copy link

@ggggggggg ggggggggg Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This RFC adds support for single-file bin packages in cargo. Single-file bin packages are .rs files with an embedded manifest and a main. cargo will be modified to accept cargo .rs as a shortcut to cargo run --manifest-path .rs; this allows placing cargo in a #! line for directly running these files."

Above I've deleted the 3rd sentence. The above paragraph communicates clearly to me the goal of being able to type cargo myfile.rs and have it do something useful. I have no idea what the 3rd sentence is trying to add. I suspect it makes sense only if you are aware of some cargo internal details. Specifically the phrase "with just like X" makes no sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change that tries to clarify that sentence:

These files will be accepted by cargo commands as --manifest-path just like Cargo.toml files.

Removing things isn't always the best solution when they aren't clear. This isn't focused on some kind of cargo internals but core aspects of how you interact with it. This is saying that these files are interchangeable with Cargo.toml and you can run commands like cargo test --manifest-path foo.rs.

text/3502-cargo-script.md Show resolved Hide resolved
text/3502-cargo-script.md Outdated Show resolved Hide resolved
The extension is optional and I don't want to detract from the point of
these being source files.t
@bstrie
Copy link
Contributor

bstrie commented Sep 26, 2023

The frontmatter syntax wouldn't need to be added to Rust's grammar, correct? In other words, the frontmatter parsing would be a preprocessing stage solely within Cargo, which would strip out the frontmatter before passing it to rustc?

@jhpratt
Copy link
Member

jhpratt commented Sep 26, 2023

I think it should be forbidden to publish a script crate without an edition specified. Otherwise we're inevitably looking at a crate that builds when it's published and fails to build later on.

@epage
Copy link
Contributor Author

epage commented Sep 26, 2023

The frontmatter syntax wouldn't need to be added to Rust's grammar, correct? In other words, the frontmatter parsing would be a preprocessing stage solely within Cargo, which would strip out the frontmatter before passing it to rustc?

This is what cargo master is doing but

  • It is no longer rust which is a priority
  • Error messages, cargo-metadata, etc will expose the lie and I do not want to hack around every case.

@epage
Copy link
Contributor Author

epage commented Sep 26, 2023

I think it should be forbidden to publish a script crate without an edition specified. Otherwise we're inevitably looking at a crate that builds when it's published and fails to build later on.

Publish is out of scope for this RFC. There are other aspects of the design that will play into this that a conversation will need to consider. For example, I expect it to turn it into a mult-file package which will require setting the edition.

@jhpratt
Copy link
Member

jhpratt commented Sep 26, 2023

Publish is out of scope for this RFC

I now see that note towards the top. It was the mention of publish = false that left me concerned for the reason stated.

@bstrie
Copy link
Contributor

bstrie commented Sep 27, 2023

This is what cargo master is doing but

Does the "but" here indicate that the goal would be to get this syntax into Rust's grammar? If so, then I'd like to consider the alternative of stuffing this data into comments rather than introducing new syntax.

Example:
````rust
#!/usr/bin/env cargo
```cargo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo has been specified in the shebang above already. Why does the user need to specify it twice? It could just allow a naked ``` fence instead (at least if a shebang is present). We know that we are in a cargo-script file in that instance, so we have the freedom to set a "default" case for what to do with fences that have no specifier.

IMO defaulting them to the manifest does not paint us in a corner while saving us character overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a #3503 question. I have it noted as a future possibility to make cargo the default (forgot to put that in originally). We need to positively identify the frontmatter format. We can make cargo the default but we wanted to start with the more strict syntax and loosen it over time as we can't go back the other direction. As for inferring it from the shebang ... I'm trying to avoid being in the business of parsing shebangs (there is a stackoverflow about what perl does for this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying it leaves us much more room for future expansion, such as embedding other associated files or serving as frontmatter for other purposes. And it will make the job of syntax highlighters much easier.

If we want less appearance of redundancy, at the expense of more characters, we could use manifest or similar. But that doesn't seem worth it.

Copy link
Member

@est31 est31 Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start choice tends to stick around. In response to my suggestion in rust-lang/cargo#12301 , I have seen concerns that it's an unfamiliar way of specifying the edition. compare it to the PR from when the edition mechanism was introduced, there one of the comments against was into a similar direction as the reason you give why cargo defaulting is now rejected: that we don't know if we want to make 2018-pre a thing or not. Not that I agree with them but a similar effect might occur if we require the cargo specifier to be present.

Regarding whether it should be discussed in #3503 or not, IMO that RFC should not be as restricted to precisely what cargo needs, but be more generic. Ideally when we add support for non manifest embeddings, we shouldn't have to make another lang RFC but handle it wholly inside cargo-script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally when we add support for non manifest embeddings, we shouldn't have to make another lang RFC

i doubt we'd need an RFC; likely a change proposal would do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3503 was merged which has us make the infostring optional

@epage
Copy link
Contributor Author

epage commented Sep 27, 2023

This is what cargo master is doing but

Does the "but" here indicate that the goal would be to get this syntax into Rust's grammar? If so, then I'd like to consider the alternative of stuffing this data into comments rather than introducing new syntax.

#3503 is the RFC for the syntax, as referred to in the PR description and in the embedding syntax section of rationale.

This solution was what we came up with in discussions with a subset of the cargo team, rust educators, and the language team. It was started and finished on zulip with the middle being handled at RustConf.

text/3502-cargo-script.md Outdated Show resolved Hide resolved
@epage
Copy link
Contributor Author

epage commented Sep 27, 2023

I now see that note towards the top. It was the mention of publish = false that left me concerned for the reason stated.

Ah, yes, I missed some sections when removing publish support from the RFC.

This was from when the proposal had us always run with `--quiet`.
epage and others added 4 commits May 6, 2024 04:31
text/3502-cargo-script.md Show resolved Hide resolved
text/3502-cargo-script.md Show resolved Hide resolved
@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels May 7, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented May 7, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@jwnrt
Copy link

jwnrt commented May 7, 2024

I'm not sure if this is worth raising here but I just started trying the frontmatter syntax and had the following problem:

The current syntax of a script does not allow comments between the shebang line and the frontmatter since this would need more sophisticated parsing to handle. Some of the projects I contribute to require a copyright/license header in a comment at the top of the file (also after an optional shebang).

I can understand this not being considered an issue since (A) the copyright comments are a constraint introduced by external projects and not intrinsic to Rust, (B) it's more of a cultural problem than a technical one and could in theory be waived by linting tools (e.g. REUSE allows copyright headers to go anywhere), and (C) the scripts can be converted to full Cargo packages if checked into a repo with such a requirement.

Still, it would be nice to be able to have a copyright header at the top of cargo-scripts.

@gmorenz
Copy link

gmorenz commented May 7, 2024

@jwnrt

Would the licenses allow for

#!/usr/bin/env cargo
---
# License Goes here.
# In a TOML comment.

[package]
...

?

@jwnrt
Copy link

jwnrt commented May 7, 2024

If the linting script is changed to ignore --- I think that's the closest you could get, yeah, thanks

@coolreader18
Copy link

coolreader18 commented May 9, 2024

Coming from TDCIC (This Development Cycle in Cargo) - wrt choosing the profile to run a script with, what about the suggested run profile, but called script? That way there's no need to change the behavior of cargo run, and it's clear what it's for.

---
[profile.script]
inherits = "release"
---

@epage
Copy link
Contributor Author

epage commented May 10, 2024

In the spirit of keeping this focused, so long as we can make a change later, let's do that rather than ensuring this RFC has every feature in it.

@ijackson
Copy link

I came here via TWIR, the blog post and epage/cargo-script-mvs#154 (comment). I have two concerns.

Firstly, conflating the script filename namespace with the cargo subcommand namespace is IMO a serious mistake. There's extensive discussion in the RFC about how to handle it, but there is a much better answer with prior art from many other script interpreters:

cargo should require an option to indicate that the argument is a script name. So the shebang should be #!/usr/bin/cargo --bikeshed or #!/usr/bin/env -S cargo --bikeshed.

Prior art:

  • To write a script for jq, use the -f/--from-file option. (#!/usr/bin/jq -f) The default is to treat the argument as jq filter text (ie, source code in the jq language, rather than a filename). awk has similar behaviour.
  • To run a file containing Emacs Lisp as a script, use emacs --script. So #!/usr/bin/emacs -script. (The default behaviour is of course to run emacs as an interactive text editor and treat the argument as a file to edit.)
  • Debian's debian/rules is a Makefile. make's default behaviour is to treat the arguments as targets and look for a Makefile in the cwd (which is analogous to cargo's default behaviour). So, debian/rules starts #! /usr/bin/make -f.
  • To run a file containing a PostgreSQL script, use psql -f, since the arguments are the database name etc.
  • If you want to write a #! script for execution by gdb, you must write #!/usr/bin/gdb -x or similar. (By default the argument is the program to debug.)

I'm sure there are many other examples.

@ijackson
Copy link

My second concern is:

The .rs extension should be optional.

Encoding the implementation language of a script in its filename is an antipattern (notwithstanding that it seems to be extremely common).

The name of a script is how you invoke it. So it is the script's API (along, of course, with its behaviour). The language its written in is an implementation detail.

This is not merely a theoretical problem of principle. It causes practical difficulties. It is very common to start out with a script in a simple language, and then to rewrite it in a more sophisticated language as the demands on it grow.

If the filename mentions the implementation language, this can't be done without updating every call site, possibly in different repositories, or even public documentation, and possibly teaching many humans to run a different command.

In the case of cargo script, insisting on a .rs extension makes it impossible to replace a script foo that once started #!/bin/bash and now starts #!/usr/bin/env python3, with a Rust implementation.

I don't think this is a blocker for the cargo script feature, since the .rs restriction could always be relaxed later. But it is a significant limitation that ought to be discussed in the RFC. (The RFC text does discuss possible other extensions, and seems to have been written on the (false) assumption that an extension is necessary at all.)

@bjorn3
Copy link
Member

bjorn3 commented May 15, 2024

So the shebang should be #!/usr/bin/cargo --bikeshed or #!/usr/bin/env -S cargo --bikeshed.

macOS doesn't support -S as noted in the "Naming" section. #!/usr/bin/cargo doesn't work with non-system installs of cargo as is the most common.

@epage
Copy link
Contributor Author

epage commented May 15, 2024

@ijackson

  • As mentioned env -S is not universal as covered in the RFC
  • At least for myself, that prior art feels "obscure". I have never written or edited a file with a shebang like in those cases.

- The command that is run in a `#!` line should not require arguments (e.g. not
`#!/usr/bin/env cargo <something>`) because it will fail. `env` treats the
rest of the line as the bin name, spaces included. You need to use `env -S`
but that wasn't supported on macOS at least, last I tested.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of decision is based around "env -S is not standard in particular macOS", but is it actually verified? @epage

  1. On latest macOS Sonoma 14.4.1, /usr/bin/env does support an -S flag. 1
  2. I have also got a macOS Big Sur 11.7.10 (2020) and its env also has the -S flag.
  3. According to Julia language's FAQ, macOS supported env -S since Sierra 10.12 (2016), and Sierra is the minimum supported macOS version since Rust 1.74.

Yes it is indeed still awkward to require the -S flag, but outright denying any alternatives that requires -S is not reasonable either.

Footnotes

  1. Additionally, unlike Linux, even skipping the -S flag it works just fine:

    #!/usr/bin/env cargo +nightly -Zscript
    fn main() { println!("works!"); }
    

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the text to not mention macOS. I was going off of memory when I did that (it was a problem at a prior job and most problems came from macOS). I have no idea if it was due to our Docker base images or something about macOS pre-Sierra.

I did confirm that busybox does not support -S. The broader point is that its not portable.

text/3502-cargo-script.md Outdated Show resolved Hide resolved
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 17, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented May 17, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@ehuss
Copy link
Contributor

ehuss commented May 17, 2024

Huzzah! The @rust-lang/cargo team has decided to accept this RFC.

To track further status updates, subscribe to the tracking issue here: rust-lang/cargo#12207

@ehuss ehuss merged commit 72ba5cc into rust-lang:master May 17, 2024
@kornelski
Copy link
Contributor

I expected ```cargo to be supported at the top level of the file, not just inside a doc comment (the file doesn't start with a valid Rust syntax, so why not break it all the way).

At least it needs a better error message:

error: unknown start of token: `
 --> script.rs:3:1
  |
3 | ```cargo
  | ^^^
  |
  = note: character appears 2 more times
help: Unicode character '`' (Grave Accent) looks like ''' (Single Quote), but it is not

@epage
Copy link
Contributor Author

epage commented Sep 1, 2024

Its triple dashes, not triple backticks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
Archived in project
Status: Unreviewed
Development

Successfully merging this pull request may close these issues.