feat: Import cargo-add into cargo by epage · Pull Request #10472 · rust-lang/car...

Motivation

The reasons I'm aware of are:

• Large interest, see #5586
• Make it easier to add a dependency when you don't care about the version (instead of having to find it or just using the major version if thats all you remember)
• Provide a guided experience, including
  • Catch or prevent errors earlier in the process
  • Bring the Manifest format documentation into the terminal via cargo add --help
  • Using version and path for dependencies but path only for dev-dependencies (see crate-ci/cargo-release#288 which led to killercup/cargo-edit#480)

Drawbacks

1. This is another area of consideration for new RFCs, like rust-lang/rfcs#3143 (this PR supports it) or rust-lang/rfcs#2906 (implementing it will require updating cargo-add)

2. This is a high UX feature that will draw a lot of attention (ie Issue influx)

We've tried to reduce the UX influx by focusing the scope to preserving semantic information (custom sort order, comments, etc) but being opinionated on syntax (style of strings, etc)

Behavior

Help output

Example commands

cargo add regex
cargo add regex serde
cargo add regex@1
cargo add regex@~1.0
cargo add --path ../dependency

For an exhaustive set of examples, see tests and associated snapshots

Particular points

• Effectively there are two modes
  • Fill in any relevant field for one package
  • Add multiple packages, erroring for fields that are package-specific (--rename)
  • Note that --git and --path only accept multiple packages from that one source
• We infer if the dependencies table is sorted and preserve that sorting when adding a new dependency
• Adding a workspace dependency
  • dev-dependencies always use path
  • all other dependencies use version + path
• Behavior is idempotent, allowing you to run cargo add serde serde_json -F serde/derive safely if you already had a dependency on serde but without serde_json
• When a registry dependency's version req is unspecified, we'll first reuse the version req from another dependency section in the manifest. If that doesn't exist, we'll use the latest version in the registry as the version req

Additional decisions

Accepting the proposed cargo-add as-is assumes the acceptance of the following:

• Add the -F short-hand for --features to all relevant cargo commands
• Support @ in pkgids in other commands where we accept :
• Add support for <name>@<version> in more commands, like cargo yank and cargo install

Alternatives

• Use : instead of @ for versions
• Flags like --features, --optional, --no-default-features would be position-sensitive, ie they would only apply to the crate immediate preceding them
  • This removes the dual-mode nature of the command and remove the need for the +feature syntax (cargo add serde -F derive serde_json)
  • There was concern over the rarity of position-sensitive flags in CLIs for adopting it here
• Support a --sort flag to sort the dependencies (existed previously)
  • To keep the scope small, we didn't want general manifest editing capabilities
• --upgrade <POLICY> flag to choose constraint (existed previously)
  • The flag was confusing as-is and we feel we should instead encourage people towards ^
• --allow-prerelease so a cargo add clap can choose among pre-releases as well
  • We felt the pre-release story is too weak in cargo-generally atm for making it first class in cargo-add
• Offer cargo add serde +derive serde_json as a shorthand
• Infer path from a positional argument

Prior Art

• (Python) poetry add
  • git+ is needed for inferring git dependencies, no separate --git flags
  • git branch is specified via a URL fragment, instead of a --branch
• (Javascript) yarn add
  • name@data where data can be version, git (with fragment for branch), etc
  • -E / --exact, -T / --tilde, -C / --caret to control version requirement operator instead of --upgrade <policy> (also controlled through defaultSemverRangePrefix in config)
  • --cached for using the lock file (killercup/cargo-edit#41)
  • In addition to --dev, it has --prefer-dev which will only add the dependency if it doesn't already exist in dependencies as well as dev-dependencies
  • --mode update-lockfile will ensure the lock file gets updated as well
• (Javascript) pnpm-add
• (Javascript) npm doesn't have a native solution
  • Specify version with @<version>
  • Also overloads <name>[@<version>] with path and repo
    • Supports a git host-specific protocol for shorthand, like github:user/repo
    • Uses fragment for git ref, seems to have some kind of special semver syntax for tags?
  • Only supports --save-exact / -E for operators outside of the default
• (Go) go get
  • Specify version with @<version>
  • Remove dependency with @none
• (Haskell) stack doesn't seem to have a native solution
• (Julia) pkg Add
• (Ruby) bundle add
  • Uses --version / -v instead of --vers (we use --vers because of --version / -V)
  • --source instead of path (path correlates to manifest field)
  • Uses --git / --branch like cargo-add
• (Dart) pub add
  • Uses --git-url instead of --git
  • Uses --git-ref instead of --branch, --tag, --rev

Future Possibilities

• Update lock file accordingly
• Exploring the idea of a --local flag
• Take the MSRV into account when automatically creating version req (killercup/cargo-edit#587)
• Integrate rustsec to report advisories on new dependencies (killercup/cargo-edit#512)
• Integrate with licensing to report license, block add, etc (e.g. killercup/cargo-edit#386)
• Pull version from lock file (killercup/cargo-edit#41)
• Exploring if any vendoring integration would be beneficial (currently errors)
• Upstream cargo-rm (#10520), cargo-upgrade (#10498), and cargo-set-version (in that order of priority)
• Update crates.io with cargo add snippets in addition to or replacing the manifest snippets

For more, see https://github.com/killercup/cargo-edit/issues?q=is%3Aissue+is%3Aopen+label%3Acargo-add

How should we test and review this PR?

This is intentionally broken up into several commits to help reviewing

1. Import of production code from cargo-edit's merge-add branch, with only changes made to let it compile (e.g. fixing up of use statements).
2. Import of test code / snapshots. The only changes outside of the import were to add the snapbox dev-dependency and to mod cargo_add into the testsuite
3. This extends the work in #10425 so I could add back in the color highlighting I had to remove as part of switching cargo-add from direct termcolor calls to calling into Shell

Structure-wise, this is similar to other commands

• bin only defines a CLI and adapts it to an AddOptions
• ops contains a focused API with everything buried under it

The "op" contains a directory, instead of just a file, because of the amount of content. Currently, all editing code is contained in there. Most of this will be broken out and reused when other cargo-edit commands are added but holding off on that for now to separate out the editing API discussions from just getting the command in.

Within the github UI, I'd recommend looking at individual commits (and the merge-add branch if interested), skipping commit 2. Commit 2 would be easier to browse locally.

cargo-add is mostly covered by end-to-end tests written using snapbox, including error cases.

There is additional cleanup that would ideally happen that was excluded intentionally from this PR to keep it better scoped, including

• Consolidating environment variables for end-to-end tests of cargo
• Pulling out the editing API, as previously mentioned
  • Where the editing API should live (cargo::edit?)
  • Any more specific naming of types to reduce clashes (e.g. Dependency or Manifest being fairly generic).
• Possibly sharing SourceId creation between cargo install and cargo edit
• Explore using snapbox in more of cargo's tests

Implementation justifications:

• dunce and pathdiff dependencies: needed for taking paths relative to the user and make them relative to the manifest being edited
• indexmap dependency (already a transitive dependency): Useful for preserving uniqueness while preserving order, like with feature values
• snapbox dev-dependency: Originally it was used to make it easy to update tests as the UX changed in prep for merging but it had the added benefit of making some UX bugs easier to notice so they got fixed. Overall, I'd like to see it become the cargo-agnostic version of cargo-test-support so there is a larger impact when improvements are made
• parse_feature function: CliFeatures forces items through a BTreeSet, losing the users specified order which we wanted to preserve.

Additional Information

See also the internals thread.

Fixes #5586