Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
src/bin/cargo/commands/run.rs
Outdated
| match manifest_path.is_file() { | ||
| true => {} |
There was a problem hiding this comment.
Cleaned up in the next commit
| // TODO: remove once frontmatter hits nightly | ||
| if unit.pkg.manifest().is_embedded() { |
There was a problem hiding this comment.
Blocked on rust-lang/rust#148051 hitting nightly
| // TODO: remove once frontmatter hits nightly | ||
| if unit.pkg.manifest().is_embedded() { |
There was a problem hiding this comment.
Blocked on rust-lang/rust#148051 hitting nightly
|
@rfcbot fcp merge T-cargo For me, my biggest concerns are the first two Risks listed. |
|
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
There was a problem hiding this comment.
Should we inform rustfmt team that we're going to stabilize this and leave frontmatter style issue unresolved? I remembered that let-else has caused confusions and frustrations when we stabilized it in the language syntax side but rustfmt didn't format it: rust-lang/rustfmt#4914.
At least the rustfmt team should be aware of the potential incoming volume of issues regarding this.
| in the [TOML] format. It contains metadata that is needed to compile the package. Checkout | ||
| the `cargo locate-project` section for more detail on how cargo finds the manifest file. | ||
|
|
||
| The content of the manifest can also be embedded inside of the frontmatter inside of a Rust source file: |
There was a problem hiding this comment.
Not a blocker but not satisfied with the current stable doc. I would love to see a dedicated section for Cargo scripts with some examples, and all other mentions of "script" to link back to that section. Currently it is a bit not obvious when it is mentioned.
The original unstable docs has some good stuff in it but maybe it depends on many stuff we want to commit as "stable"
(Or it was migrated to elsewhere? The diff is too large that I could have missed it)
There was a problem hiding this comment.
Was unsure how to handle it. A guide? If so, where does it fit in. A reference? Does it get its own page or a section on manifests?
There was a problem hiding this comment.
Or a reference-like doc in src/doc/man/cargo.md, and we have another standalone guide? In manifest is not too bad but script is more than manifest.
All these teams actually |
This is worth calling out in user doc that whether the target directory of script is considered implementation details. |
This was handled as part of the Frontmatter stabilization. |
| in the [TOML] format. It contains metadata that is needed to compile the package. Checkout | ||
| the `cargo locate-project` section for more detail on how cargo finds the manifest file. | ||
|
|
||
| The content of the manifest can also be embedded inside of the frontmatter inside of a Rust source file: |
There was a problem hiding this comment.
Or a reference-like doc in src/doc/man/cargo.md, and we have another standalone guide? In manifest is not too bad but script is more than manifest.
### What does this PR try to resolve? This is to make space for discussing scripts, see #16569 ### How to test and review this PR?
|
If we stabilise This is subjective, but it feels to me like an "impl trait in argument position" situation where a more controversial extension of a feature is slipping into the stabilisation of an overwhelmingly popular feature. I wonder whether it might be best to omit this from the initial stabilisation. |
|
|
There was a problem hiding this comment.
IIUC, it's not possible to have a script as a member of workspace. Is that correct? Is there a strategy for allowing that in the future?
For now, what is the intended way for users to clean the build directories? Is it cargo clean --manifest-path=script.rs?
| - The file name is `Cargo.toml` | ||
|
|
||
| Differences between `cargo run --manifest-path <path>` and `cargo <path>` | ||
| - `cargo <path>` runs with the config for `<path>` and not the current dir, more like `cargo install --path <path>` |
There was a problem hiding this comment.
I'm a little confused on this. This says that cargo <path> loads the config from the given path. But the PR description says it loads from CARGO_HOME. Which is it?
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
This comment has been minimized.
This comment has been minimized.
Workspace support is deferred out. My plan had been for us to check files among workspace membes but that was before I knew about #8511 as my last knowledge on this was that cargo errored for anything that matched the glob that wasn't a valid. I don't see this affecting us now but it does mean that more work will be needed to figure out workspace support
Yes. There is an issue for us to GC unused |
|
For workspaces, a concern I had was about auto-discovery. If in the future we decide to support workspaces and cargo did auto-discovery if a script is inside a workspace, then that would be a change that could be difficult to transition. It would be incompatible with older versions, and would cause If it is intended to only be opt-in (like via a frontmatter field), then that should be fine. I'm not sure if there are downsides with that. Opt-in via I just wanted to make sure we don't accidentally lock ourselves into a particular solution we didn't plan for. |
My assumption has been that we will not auto-discover the workspace for a script but instead users will need to opt-in to a workspace through |
|
What is (or will be) the intended behavior of |
This comment has been minimized.
This comment has been minimized.
I posted #16633 to make the existing behavior explicit. My comment at #16569 (comment) about #8511 equally applies to #16611
We'll need some way to mention them as workspace members. Off the top of my head, some ideas I came up with are
I had gone into all of this with pre- #8511 knowledge and #8511 and #16611 dramatically change what we can do. |
8c6298b to
e1bbe62
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
### What does this PR try to resolve? Inspired by the conversation at #16569 (comment) ### How to test and review this PR?
|
☔ The latest upstream changes (possibly ba739a5) made this pull request unmergeable. Please resolve the merge conflicts. |
Fixes #12207
RFC: https://rust-lang.github.io/rfcs/3502-cargo-script.html
Tracking issue: #12207
Frontmatter stabilization: rust-lang/rust#148051
This stabilizes an MVP of Cargo script
.rsextensionCargo.tomlpackage.nameis optional, defaults to the file stempackage.editiondefaults to current edition, not 2015 edition, with a warningpackage.build,package.links,package.default-run,package.workspace,package.auto-*are disallowedworkspace,build-dependencies, and any build-target tables are disallowedpackage.namedefaulting logicpackageUnicodeXIDcharacters with-or_contestselfdepscargo fooprecedence:.rsextension or a path component is present (e.g../foo)cargo installerrors on publishing a scriptcargo packageerrors on publishing a scriptcargo publisherrors on publishing a scriptarg0is set on a best-effort basis to the script's pathCARGO_MANIFEST_PATHwas previously stabilized in addition to the existingCARGO_MANIFEST_DIRcargo foo.rsloads config relative tofoocargo install --path bar/and unlike the rest of the Cargo commandscargo run --manifest-path foo.rsloads from current dirWhats missing:
Known issues:
cargo removeprints the "unset edition" warning twice#!and[]and not commentsbuild-dir. Garbage collect wholetarget/#13136 will resolve this.Risks
If people share their(fix(script): Make the lockfile script-specific independent of build-dir #16619)build-diracross projects, then multiple scripts will point to the sameCargo.lock, fighting over what is locked in itNot allowing local config for scripts could become a problem as we get into workspace support as people would expect their repo's config to be respected when it won't be(fix(script): Load config relative to the script #16620)cargo fooPATHthat makes this not workFuture possibilities
target/#13136)package.nameoptional #12689cargo Cargo.tomlsupport