Expose cargo add internals as edit API #11059
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
Specifically, I wanted to separate out the conversation on "where" from actually merging |
epage
reviewed
Sep 7, 2022
|
☔ The latest upstream changes (presumably #11075) made this pull request unmergeable. Please resolve the merge conflicts. |
weihanglo
reviewed
Sep 13, 2022
Move cargo add utils out for future use by other subcommands (namely cargo remove, per rust-lang#10520).
cassaundra
force-pushed
the
edit-refactor
branch
from
67491dd
to
dd5d61c
Compare
Yes. I just resolved the merge conflicts introduced by #11075, and will next add more module documentation to |
weihanglo
added
the
A-cargo-api
|
I've added better module documentation for |
Notable changes: - Add/improve module documentation. - Add missing item documentation. - Add "editable" to `Dependency` and `Manifest` to further distinguish them from similarly named items in the library. Stylistic changes: - Terminate doc comments with a period. - Wrap doc comments.
cassaundra
force-pushed
the
edit-refactor
branch
from
64d57b8
to
d1b041d
Compare
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
14 tasks
5 tasks
weihanglo
added a commit
to weihanglo/rust
that referenced
this pull request
Sep 18, 2022
8 commits in 082503982ea0fb7a8fd72210427d43a2e2128a63..73ba3f35e0205844418260722c11602113179c4a 2022-09-13 17:49:38 +0000 to 2022-09-18 06:38:16 +0000 - Revert "Clarify when cargo detects changes" (rust-lang/cargo#11107) - Fix links to workspace inheritance headings in workspace docs (rust-lang/cargo#11103) - docs(ref): Clarify workspace settings (rust-lang/cargo#11082) - Update comment about ResolveVersion default version (rust-lang/cargo#11095) - [master] Run `reach_max_unpack_size` test only on debug build (rust-lang/cargo#11091) - Clarify when cargo detects changes (rust-lang/cargo#11092) - [master] Fix for CVE-2022-36113 and CVE-2022-36114 (rust-lang/cargo#11089) - Expose cargo add internals as edit API (rust-lang/cargo#11059)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 19, 2022
Update cargo (CVE fixes included) 8 commits in 082503982ea0fb7a8fd72210427d43a2e2128a63..73ba3f35e0205844418260722c11602113179c4a 2022-09-13 17:49:38 +0000 to 2022-09-18 06:38:16 +0000 - Revert "Clarify when cargo detects changes" (rust-lang/cargo#11107) - Fix links to workspace inheritance headings in workspace docs (rust-lang/cargo#11103) - docs(ref): Clarify workspace settings (rust-lang/cargo#11082) - Update comment about ResolveVersion default version (rust-lang/cargo#11095) - [master] Run `reach_max_unpack_size` test only on debug build (rust-lang/cargo#11091) - Clarify when cargo detects changes (rust-lang/cargo#11092) - [master] Fix for GHSA-rfj2-q3h3-hm5j and GHSA-2hvr-h6gw-qrxp (rust-lang/cargo#11089) - Expose cargo add internals as edit API (rust-lang/cargo#11059)
bors
added a commit
that referenced
this pull request
Oct 6, 2022
Import `cargo remove` into cargo ## What does this PR try to resolve? This PR merges `cargo remove` from [cargo-edit](https://github.com/killercup/cargo-edit) into cargo. ### Motivation - General approval from community, see #5586 and #10520. - Satisfying symmetry between add and remove. - Help users clean up their manifests (for example, when users forget to remove optional dependencies from feature lists). With #10472, cargo-add was added to cargo. As part of that discussion, it was also proposed that `cargo rm` (now `cargo remove`) eventually be added as well. ### Drawbacks - Additional code always opens the door for more bugs and features - The scope of this command is fairly small though - Known bugs and most known features were resolved before this merge proposal ### Behavior `cargo remove` operates on one or more dependencies from a manifest, removing them from a specified dependencies section (using the same flags as `cargo-add`) and from `[features]` activations if the dependency is optional. Feature lists themselves are not automatically removed when made empty. Like with cargo-add, the lock file is automatically updated. Note: like `cargo add`, `cargo remove` refers to dependency names, rather than crate names, which can be different with the presence of the `name` field. Note: `cargo rm` has been renamed to `cargo remove`, based on prior art and user feedback (see [discussion](#10520)). Although this renaming is arguably an improvement, adding an `rm` alias could make the switch easier for existing users of cargo-edit (at the cost of a naming conflict which would merit insta-stabilization). #### Help output <details> ```shell $ cargo run -- remove --help cargo-remove Remove dependencies from a Cargo.toml manifest file USAGE: cargo remove [OPTIONS] <DEP_ID>... ARGS: <DEP_ID>... Dependencies to be removed OPTIONS: -p, --package [<SPEC>...] Package to remove from -v, --verbose Use verbose output (-vv very verbose/build.rs output) --manifest-path <PATH> Path to Cargo.toml --offline Run without accessing the network -q, --quiet Do not print cargo log messages --dry-run Don't actually write the manifest -Z <FLAG> Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details -h, --help Print help information SECTION: --dev Remove as development dependency --build Remove as build dependency --target <TARGET> Remove as dependency from the given target platform ``` </details> #### Example usage ``` cargo remove serde cargo remove criterion httpmock --dev cargo remove winhttp --target x86_64-pc-windows-gnu cargo remove --package core toml ``` ## How should we test and review this PR? This is following the pattern from cargo-add which was implemented in three different PRs (implementation, documentation, and completions), in the interest of reducing the focusing discussions in each PR and allowing cargo-add's behavior to settle to avoid documentation churn. 1. #10472 2. #10578 3. #10577 The remaining changes (documentation and shell completions) will follow shortly after. Some work has already begun on this feature in #11059. Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@master...merge-rm) branch of cargo-edit with PRs reviewed by `@epage.` If you are interested in seeing how this feature evolved to better match cargo's internals, you might find the commit history there to be helpful. As this PR is reviewed, changes will be made both here and on that branch, with the commit history being fully maintained on the latter. `cargo remove` is structured like most other subcommands: - `src/bin/cargo/commands/remove.rs` contains the cli handling and top-level execution. - `src/cargo/ops/cargo_remove.rs` contains the implementation of the feature itself. In order to support this feature, the `remove_from_table` util was added to `util::toml_mut::manifest::LocalManifest`. Tests are split out into a separate commit to make it easier to review the production code and tests. Tests have been implemented with `snapbox`, structured similarly to the tests of `cargo add`. ### Prior art - Python: [`poetry remove`](https://python-poetry.org/docs/cli/#remove) - Supports dry run - JavaScript: [`yarn remove`](https://yarnpkg.com/cli/remove) - Supports wildcards - JavaScript: [`pnpm remove`](https://pnpm.io/cli/remove) - Go: [`go get`](https://go.dev/ref/mod#go-get) - `go get foo@none` to remove - Julia: [`pkg rm`](https://docs.julialang.org/en/v1/stdlib/Pkg/) - Supports `--all` to remove all dependencies - Ruby: [`bundle remove`](https://bundler.io/v2.2/man/bundle-remove.1.html) - Dart: [`dart pub remove`](https://dart.dev/tools/pub/cmd/pub-remove) - Supports dry run - Lua: [`luarocks remove`](https://github.com/luarocks/luarocks/wiki/remove) - Supports force remove - .NET: [`Uninstall-Package`](https://docs.microsoft.com/en-us/nuget/reference/ps-reference/ps-ref-uninstall-package) - Supports dry run - Supports removal of dependencies - Supports force remove (disregards dependencies) - Haxe: [`haxelib remove`](https://lib.haxe.org/documentation/using-haxelib/#remove) - Racket: [`raco pkg remove`](https://docs.racket-lang.org/pkg/cmdline.html#%28part._raco-pkg-remove%29) - Supports dry run - Supports force remove (disregards dependencies) - Supports demotion to weak dependency (sort of a corollary of force remove) ### Insta-stabilization In the discussion of `cargo add`'s stabilization story ([Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add)), it was brought up that the feature might benefit from being insta-stabilized to avoid making the cargo-edit version of the binary hard to access. Since `cargo rm` (from cargo-edit) was renamed to `cargo remove` here, [such a conflict no longer exists](https://crates.io/search?q=cargo%20remove), so this is less of a concern. Since this feature is already has a had a long run of user testing in cargo-edit and doesn't have unsettled UI questions like cargo-add did, it might still be a candidate for insta-stabilization. ### Deferred work Necessary future work: - Add documentation. - Add shell completions. - Perform GC on workspace dependencies when they are no longer used (see #8415). - This is inspired by a feature from the RFC that was dropped (unused dependencies triggering a warning) - This was deferred out to avoid challenges with testing nightly features It was found in the review of `cargo add` that it was best to defer these first two items to focus the discussion and as there was still behavior churn during the review of cargo-add. ### Future Possibilities The following are features which we might want to add to `cargo remove` in the future: - Add a `cargo rm` alias to ease transition for current cargo-edit users - Automatically convert between dash and underscores in deps: killercup/cargo-edit#690 - Remove unused dependencies: killercup/cargo-edit#415 - Clean up caches: killercup/cargo-edit#647 ### Additional information Fixes #10520.
bors
added a commit
that referenced
this pull request
Oct 6, 2022
Import `cargo remove` into cargo ## What does this PR try to resolve? This PR merges `cargo remove` from [cargo-edit](https://github.com/killercup/cargo-edit) into cargo. ### Motivation - General approval from community, see #5586 and #10520. - Satisfying symmetry between add and remove. - Help users clean up their manifests (for example, when users forget to remove optional dependencies from feature lists). With #10472, cargo-add was added to cargo. As part of that discussion, it was also proposed that `cargo rm` (now `cargo remove`) eventually be added as well. ### Drawbacks - Additional code always opens the door for more bugs and features - The scope of this command is fairly small though - Known bugs and most known features were resolved before this merge proposal ### Behavior `cargo remove` operates on one or more dependencies from a manifest, removing them from a specified dependencies section (using the same flags as `cargo-add`) and from `[features]` activations if the dependency is optional. Feature lists themselves are not automatically removed when made empty. Like with cargo-add, the lock file is automatically updated. Note: like `cargo add`, `cargo remove` refers to dependency names, rather than crate names, which can be different with the presence of the `name` field. Note: `cargo rm` has been renamed to `cargo remove`, based on prior art and user feedback (see [discussion](#10520)). Although this renaming is arguably an improvement, adding an `rm` alias could make the switch easier for existing users of cargo-edit (at the cost of a naming conflict which would merit insta-stabilization). #### Help output <details> ```shell $ cargo run -- remove --help cargo-remove Remove dependencies from a Cargo.toml manifest file USAGE: cargo remove [OPTIONS] <DEP_ID>... ARGS: <DEP_ID>... Dependencies to be removed OPTIONS: -p, --package [<SPEC>...] Package to remove from -v, --verbose Use verbose output (-vv very verbose/build.rs output) --manifest-path <PATH> Path to Cargo.toml --offline Run without accessing the network -q, --quiet Do not print cargo log messages --dry-run Don't actually write the manifest -Z <FLAG> Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details -h, --help Print help information SECTION: --dev Remove as development dependency --build Remove as build dependency --target <TARGET> Remove as dependency from the given target platform ``` </details> #### Example usage ``` cargo remove serde cargo remove criterion httpmock --dev cargo remove winhttp --target x86_64-pc-windows-gnu cargo remove --package core toml ``` ## How should we test and review this PR? This is following the pattern from cargo-add which was implemented in three different PRs (implementation, documentation, and completions), in the interest of reducing the focusing discussions in each PR and allowing cargo-add's behavior to settle to avoid documentation churn. 1. #10472 2. #10578 3. #10577 The remaining changes (documentation and shell completions) will follow shortly after. Some work has already begun on this feature in #11059. Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@master...merge-rm) branch of cargo-edit with PRs reviewed by `@epage.` If you are interested in seeing how this feature evolved to better match cargo's internals, you might find the commit history there to be helpful. As this PR is reviewed, changes will be made both here and on that branch, with the commit history being fully maintained on the latter. `cargo remove` is structured like most other subcommands: - `src/bin/cargo/commands/remove.rs` contains the cli handling and top-level execution. - `src/cargo/ops/cargo_remove.rs` contains the implementation of the feature itself. In order to support this feature, the `remove_from_table` util was added to `util::toml_mut::manifest::LocalManifest`. Tests are split out into a separate commit to make it easier to review the production code and tests. Tests have been implemented with `snapbox`, structured similarly to the tests of `cargo add`. ### Prior art - Python: [`poetry remove`](https://python-poetry.org/docs/cli/#remove) - Supports dry run - JavaScript: [`yarn remove`](https://yarnpkg.com/cli/remove) - Supports wildcards - JavaScript: [`pnpm remove`](https://pnpm.io/cli/remove) - Go: [`go get`](https://go.dev/ref/mod#go-get) - `go get foo@none` to remove - Julia: [`pkg rm`](https://docs.julialang.org/en/v1/stdlib/Pkg/) - Supports `--all` to remove all dependencies - Ruby: [`bundle remove`](https://bundler.io/v2.2/man/bundle-remove.1.html) - Dart: [`dart pub remove`](https://dart.dev/tools/pub/cmd/pub-remove) - Supports dry run - Lua: [`luarocks remove`](https://github.com/luarocks/luarocks/wiki/remove) - Supports force remove - .NET: [`Uninstall-Package`](https://docs.microsoft.com/en-us/nuget/reference/ps-reference/ps-ref-uninstall-package) - Supports dry run - Supports removal of dependencies - Supports force remove (disregards dependencies) - Haxe: [`haxelib remove`](https://lib.haxe.org/documentation/using-haxelib/#remove) - Racket: [`raco pkg remove`](https://docs.racket-lang.org/pkg/cmdline.html#%28part._raco-pkg-remove%29) - Supports dry run - Supports force remove (disregards dependencies) - Supports demotion to weak dependency (sort of a corollary of force remove) ### Insta-stabilization In the discussion of `cargo add`'s stabilization story ([Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add)), it was brought up that the feature might benefit from being insta-stabilized to avoid making the cargo-edit version of the binary hard to access. Since `cargo rm` (from cargo-edit) was renamed to `cargo remove` here, [such a conflict no longer exists](https://crates.io/search?q=cargo%20remove), so this is less of a concern. Since this feature is already has a had a long run of user testing in cargo-edit and doesn't have unsettled UI questions like cargo-add did, it might still be a candidate for insta-stabilization. ### Deferred work Necessary future work: - Add documentation. - Add shell completions. - Perform GC on workspace dependencies when they are no longer used (see #8415). - This is inspired by a feature from the RFC that was dropped (unused dependencies triggering a warning) - This was deferred out to avoid challenges with testing nightly features It was found in the review of `cargo add` that it was best to defer these first two items to focus the discussion and as there was still behavior churn during the review of cargo-add. ### Future Possibilities The following are features which we might want to add to `cargo remove` in the future: - Add a `cargo rm` alias to ease transition for current cargo-edit users - Automatically convert between dash and underscores in deps: killercup/cargo-edit#690 - Remove unused dependencies: killercup/cargo-edit#415 - Clean up caches: killercup/cargo-edit#647 ### Additional information Fixes #10520.
bors
added a commit
that referenced
this pull request
Oct 6, 2022
Import `cargo remove` into cargo ## What does this PR try to resolve? This PR merges `cargo remove` from [cargo-edit](https://github.com/killercup/cargo-edit) into cargo. ### Motivation - General approval from community, see #5586 and #10520. - Satisfying symmetry between add and remove. - Help users clean up their manifests (for example, when users forget to remove optional dependencies from feature lists). With #10472, cargo-add was added to cargo. As part of that discussion, it was also proposed that `cargo rm` (now `cargo remove`) eventually be added as well. ### Drawbacks - Additional code always opens the door for more bugs and features - The scope of this command is fairly small though - Known bugs and most known features were resolved before this merge proposal ### Behavior `cargo remove` operates on one or more dependencies from a manifest, removing them from a specified dependencies section (using the same flags as `cargo-add`) and from `[features]` activations if the dependency is optional. Feature lists themselves are not automatically removed when made empty. Like with cargo-add, the lock file is automatically updated. Note: like `cargo add`, `cargo remove` refers to dependency names, rather than crate names, which can be different with the presence of the `name` field. Note: `cargo rm` has been renamed to `cargo remove`, based on prior art and user feedback (see [discussion](#10520)). Although this renaming is arguably an improvement, adding an `rm` alias could make the switch easier for existing users of cargo-edit (at the cost of a naming conflict which would merit insta-stabilization). #### Help output <details> ```shell $ cargo run -- remove --help cargo-remove Remove dependencies from a Cargo.toml manifest file USAGE: cargo remove [OPTIONS] <DEP_ID>... ARGS: <DEP_ID>... Dependencies to be removed OPTIONS: -p, --package [<SPEC>...] Package to remove from -v, --verbose Use verbose output (-vv very verbose/build.rs output) --manifest-path <PATH> Path to Cargo.toml --offline Run without accessing the network -q, --quiet Do not print cargo log messages --dry-run Don't actually write the manifest -Z <FLAG> Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details -h, --help Print help information SECTION: --dev Remove as development dependency --build Remove as build dependency --target <TARGET> Remove as dependency from the given target platform ``` </details> #### Example usage ``` cargo remove serde cargo remove criterion httpmock --dev cargo remove winhttp --target x86_64-pc-windows-gnu cargo remove --package core toml ``` ## How should we test and review this PR? This is following the pattern from cargo-add which was implemented in three different PRs (implementation, documentation, and completions), in the interest of reducing the focusing discussions in each PR and allowing cargo-add's behavior to settle to avoid documentation churn. 1. #10472 2. #10578 3. #10577 The remaining changes (documentation and shell completions) will follow shortly after. Some work has already begun on this feature in #11059. Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@master...merge-rm) branch of cargo-edit with PRs reviewed by `@epage.` If you are interested in seeing how this feature evolved to better match cargo's internals, you might find the commit history there to be helpful. As this PR is reviewed, changes will be made both here and on that branch, with the commit history being fully maintained on the latter. `cargo remove` is structured like most other subcommands: - `src/bin/cargo/commands/remove.rs` contains the cli handling and top-level execution. - `src/cargo/ops/cargo_remove.rs` contains the implementation of the feature itself. In order to support this feature, the `remove_from_table` util was added to `util::toml_mut::manifest::LocalManifest`. Tests are split out into a separate commit to make it easier to review the production code and tests. Tests have been implemented with `snapbox`, structured similarly to the tests of `cargo add`. ### Prior art - Python: [`poetry remove`](https://python-poetry.org/docs/cli/#remove) - Supports dry run - JavaScript: [`yarn remove`](https://yarnpkg.com/cli/remove) - Supports wildcards - JavaScript: [`pnpm remove`](https://pnpm.io/cli/remove) - Go: [`go get`](https://go.dev/ref/mod#go-get) - `go get foo@none` to remove - Julia: [`pkg rm`](https://docs.julialang.org/en/v1/stdlib/Pkg/) - Supports `--all` to remove all dependencies - Ruby: [`bundle remove`](https://bundler.io/v2.2/man/bundle-remove.1.html) - Dart: [`dart pub remove`](https://dart.dev/tools/pub/cmd/pub-remove) - Supports dry run - Lua: [`luarocks remove`](https://github.com/luarocks/luarocks/wiki/remove) - Supports force remove - .NET: [`Uninstall-Package`](https://docs.microsoft.com/en-us/nuget/reference/ps-reference/ps-ref-uninstall-package) - Supports dry run - Supports removal of dependencies - Supports force remove (disregards dependencies) - Haxe: [`haxelib remove`](https://lib.haxe.org/documentation/using-haxelib/#remove) - Racket: [`raco pkg remove`](https://docs.racket-lang.org/pkg/cmdline.html#%28part._raco-pkg-remove%29) - Supports dry run - Supports force remove (disregards dependencies) - Supports demotion to weak dependency (sort of a corollary of force remove) ### Insta-stabilization In the discussion of `cargo add`'s stabilization story ([Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add)), it was brought up that the feature might benefit from being insta-stabilized to avoid making the cargo-edit version of the binary hard to access. Since `cargo rm` (from cargo-edit) was renamed to `cargo remove` here, [such a conflict no longer exists](https://crates.io/search?q=cargo%20remove), so this is less of a concern. Since this feature is already has a had a long run of user testing in cargo-edit and doesn't have unsettled UI questions like cargo-add did, it might still be a candidate for insta-stabilization. ### Deferred work Necessary future work: - Add documentation. - Add shell completions. - Perform GC on workspace dependencies when they are no longer used (see #8415). - This is inspired by a feature from the RFC that was dropped (unused dependencies triggering a warning) - This was deferred out to avoid challenges with testing nightly features It was found in the review of `cargo add` that it was best to defer these first two items to focus the discussion and as there was still behavior churn during the review of cargo-add. ### Future Possibilities The following are features which we might want to add to `cargo remove` in the future: - Add a `cargo rm` alias to ease transition for current cargo-edit users - Automatically convert between dash and underscores in deps: killercup/cargo-edit#690 - Remove unused dependencies: killercup/cargo-edit#415 - Clean up caches: killercup/cargo-edit#647 ### Additional information Fixes #10520.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move the manifest editing utilities from cargo add to a new
cargo::util::editmodule as part of prep work forcargo remove(#10520). No other substantive changes have been made, as this PR is intended only to reduce the refactoring surface area of the implementation of the feature itself.Background
In cargo edit, there are a number of top-level modules which enable editing of Cargo manifest files (including
src/dependency.rsandsrc/manifest.rs). In #10472, these files were added instead as a submodule of the cargo add command, with the stated intention of breaking them out later for subsequentcargo-editsubcommands. This PR follows through on that expectation.Decisions
Concerns raised in #10472 regarding this change:
cargo::ops::editcargo::ops::resolveand others to have utils shared by multiple ops live incargo::ops. This is also serves to be a rather conservative API change.editcould be overly general for those unfamiliar with the cargo edit project (see alternatives)cargo::edit: this seems to me to be too top level, and would confuse users trying to discover the cargo APIcargo::util::edit: if we want to expose this at a higher level, perhaps renaming to act as a counterpart tocrate::util::tomleditwithtoml_edit,toml_mut,manifest_edit,manifest_mut,edit_toml,edit_manifestetc. for a more specific module nameDependencyorManifestbeing fairly generic)editmodule makes more clearEditDependency/EditManifest,TomlDependency/TomlManifest, etc.