-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: cargo-script #3502
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
text/3502-cargo-script.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 likeCargo.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
.
The extension is optional and I don't want to detract from the point of these being source files.t
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? |
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. |
This is what cargo master is doing but
|
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. |
I now see that note towards the top. It was the mention of |
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. |
text/3502-cargo-script.md
Outdated
Example: | ||
````rust | ||
#!/usr/bin/env cargo | ||
```cargo |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
#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. |
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`.
Co-authored-by: Josh Triplett <[email protected]>
Co-authored-by: Josh Triplett <[email protected]>
Co-authored-by: Josh Triplett <[email protected]>
Co-authored-by: Josh Triplett <[email protected]>
🔔 This is now entering its final comment period, as per the review above. 🔔 |
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. |
Would the licenses allow for
? |
If the linting script is changed to ignore |
Coming from TDCIC (This Development Cycle in Cargo) - wrt choosing the profile to run a script with, what about the suggested
|
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. |
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 Prior art:
I'm sure there are many other examples. |
My second concern is: The 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 I don't think this is a blocker for the cargo script feature, since the |
macOS doesn't support |
text/3502-cargo-script.md
Outdated
- 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. |
There was a problem hiding this comment.
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
- On latest macOS Sonoma 14.4.1,
/usr/bin/env
does support an-S
flag. 1 - I have also got a macOS Big Sur 11.7.10 (2020) and its
env
also has the-S
flag. - 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
-
Additionally, unlike Linux, even skipping the
-S
flag it works just fine:
↩#!/usr/bin/env cargo +nightly -Zscript fn main() { println!("works!"); }
There was a problem hiding this comment.
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.
Co-authored-by: kennytm <[email protected]>
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. |
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 |
I expected At least it needs a better error message:
|
Its triple dashes, not triple backticks. |
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:
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.