feat: Import cargo-add into cargo

[epage]

Motivation

The reasons I'm aware of are:

• Large interest, see Add cargo-add (from cargo-edit) to cargo proper #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 Handle package cycles crate-ci/cargo-release#288 which led to cargo add should "Do the Right Thing" for workspaces killercup/cargo-edit#480)

Drawbacks

1. This is another area of consideration for new RFCs, like Stabilize Cargo's weak-dep-features and namespaced-features. rfcs#3143 (this PR supports it) or RFC: Deduplicate Cargo workspace information 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)

e.g.

• "cargo add" should use dotted keys if already used in Cargo.toml killercup/cargo-edit#521
• cargo add: include only MAJOR.MINOR killercup/cargo-edit#126
• cargo add: use predominant string style killercup/cargo-edit#217

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

$ cargo run -- add --help    
cargo-add                                                                                                                                  [4/4594]
Add dependencies to a Cargo.toml manifest file
                                                                         
USAGE:
    cargo add [OPTIONS] <DEP>[@<VERSION>] ...
    cargo add [OPTIONS] --path <PATH> ...
    cargo add [OPTIONS] --git <URL> ...

ARGS:
    <DEP_ID>...    Reference to a package to add as a dependency

OPTIONS:
        --no-default-features     Disable the default features
        --default-features        Re-enable the default features
    -F, --features <FEATURES>     Space-separated list of features to add 
        --optional                Mark the dependency as optional
    -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
        --no-optional             Mark the dependency as required
        --color <WHEN>            Coloring: auto, always, never
        --rename <NAME>           Rename the dependency
        --frozen                  Require Cargo.lock and cache are up to date
        --manifest-path <PATH>    Path to Cargo.toml
        --locked                  Require Cargo.lock is up to date
    -p, --package <SPEC>          Package to modify
        --offline                 Run without accessing the network
        --config <KEY=VALUE>      Override a configuration value (unstable)
    -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

SOURCE:
        --path <PATH>        Filesystem path to local crate to add
        --git <URI>          Git repository location
        --branch <BRANCH>    Git branch to download the crate from
        --tag <TAG>          Git tag to download the crate from
        --rev <REV>          Git reference to download the crate from
        --registry <NAME>    Package registry for this dependency

SECTION:
        --dev                Add as development dependency
        --build              Add as build dependency
        --target <TARGET>    Add as dependency to the given target platform

EXAMPLES:
  $ cargo add regex --build
  $ cargo add trycmd --dev
  $ cargo add --path ./crate/parser/
  $ cargo add serde serde_json -F serde/derive

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 (cargo add: Be clever and infer version from Cargo.lock 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 (Take MSRV into account when auto-selecting a versio killercup/cargo-edit#587)
• Integrate rustsec to report advisories on new dependencies (Check rustsec on cargo add killercup/cargo-edit#512)
• Integrate with licensing to report license, block add, etc (e.g. Option to include license comment with crate? killercup/cargo-edit#386)
• Pull version from lock file (cargo add: Be clever and infer version from Cargo.lock killercup/cargo-edit#41)
• Exploring if any vendoring integration would be beneficial (currently errors)
• Upstream cargo-rm (Add cargo-rm (from cargo-edit) to cargo proper  #10520), cargo-upgrade (A method to update manifest version requirements (like cargo-edit's 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 feat(search): Highlight search term #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

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Awesome! Really appreciate your great work on integrating cargo-add. I have walked through half of the PR and I think a quick feedback loop is better. Here is a summary of what I've seen so far:

Thing I like

• I love how the code is organized! It is self-contained and in a good shape.
• Every public item is documented 👍🏾
• The coverage of snapshot tests is great!
• Love the write-up of long help messages and the help headings! I can foresee lots of room to improve existing commands

Things I feel uncertain

• IIRC we have a convention that the first line of warning/error messages should be lower-case.
• Some code duplications exist. Such as the logic of selecting a package (cargo-install) or SourceId creation. They might be slightly different than the other but look really similar IMO.
• Introduce more ways to get the job done. For example, dunce and snapbox. We need a non-trivial amount of work to migrate from one to another.

Personal preferences

• Use implicit format-arg capture when possible
• I would try to avoid making items/fields public unless they get value to do so.
• I feel like some structs do not need to derive Clone.

Great job and thank you @epage!

[weihanglo]

semver::Version already handles the prerelease version as a lesser one. And I get the impression that cargo install also ignores prerelease versions unless specified, though I cannot find the relevant code at this moment.

• https://docs.rs/semver/1.0.6/semver/struct.Version.html#total-ordering
• https://doc.rust-lang.org/1.59.0/cargo/reference/resolver.html#pre-releases

Suggested change

	.max_by_key(|s| {
	let stable = s.version().pre.is_empty();
	(stable, s.version())
	})
	.max_by_key(|s| s.version())

[epage]

If I'm reading this correctly:

• semver says 1.0.0 < 2.0.0-alpha.1
• cargo-add says 2.0.0-alpha.1 < 1.0.0

The goal with this code was to only select a pre-release if that is the only option available.

[weihanglo]

I am reading the semver crate again. It does state that pre-release versions will be ignored if not specified in VersionReq in the doc¹ and its code². Also, each implementation of Source::query calls Dependency::matches_id and eventually calls VersionReq::matches³. That being said,

• If prerelease is specified in VersionReq, that means the user is fine with picking prerelease version, and semver will pick them if possible.
• If not, semver just ignores them.

Maybe we needn't to check prerelease here and depend on semver for consistency?

Footnotes

1. https://docs.rs/semver/1.0.6/semver/struct.VersionReq.html#associatedconstant.STAR ↩

2. https://github.com/dtolnay/semver/blob/8d8bdb2adaf4572b3c4e8502e7498838b2cf7ccb/src/eval.rs#L3-L24 ↩

3. https://github.com/rust-lang/cargo/blob/b816d82ee94acd4d6be588c0f0cd299701c55457/src/cargo/core/source/mod.rs#L32 ↩

[weihanglo]

Maybe we can add a package with version 100000.0.0+my-package to our tests to prevent regression. The test case install::pick_max_version already did that.

[epage]

The important thing to remember is this function's intention is for us to find a version for creating a version-req. We are in the case where dep.source().is_none(). For example, if you do a cargo add clap on an empty crate, we should select 3.1.6 but no pre-release version. The user can explicitly do so with cargo add clap@3.0.0-rc.0. However, if the crate only exists as pre-release versions, we should allow the user to add that.

[epage]

Maybe we can add a package with version 100000.0.0+my-package to our tests to prevent regression. The test case install::pick_max_version already did that.

add_basic (and many tests using the common package names) currently select from

• 0.1.1
• 0.2.3
• 0.4.1
• 20.0.0
• 999999.0.0 (no idea if I have the number of 9s right)
• 999999.0.0-alpha.1

Unsure what 100000.0.0+my-package would add that isn't already handled.

I am added a test case that is pre-release only

[weihanglo]

Ah! I think I got the difference. So basically cargo-add tries harder than cargo-install on finding a usable version. cargo-install avoids prerelease version by passing a STAR VersionReq, whereas cargo-add gets a Dependency with OptVersionReq::Any which accepts any versions if unspecified.

Anyway, thanks for your explanation. I am fine with this tiny difference and it indeed makes cargo-add more convenient. 😄

[epage]

@weihanglo

Some code duplications exist. Such as the logic of selecting a package (cargo-install) or SourceId creation. They might be slightly different than the other but look really similar IMO.

I'll set aside SourceID as we have a specific conversation on that.

As for selecting a package, I'm going to assume you mean the mapping of arguments to packages

• cargo-add supports inline versions with @ and cargo-install doesn't support any inline version as far as I'm aware. Ideally this gets changed to accepting a pkgid and pkgid gets extended to supporting @. Even at that point, one difference is that cargo-add is accepting a version-req while cargo-install and cargo-yank would accept a version.
• cargo-add fills in unspecified arguments from the existing dependency entry. This makes it harder to reuse.

Introduce more ways to get the job done. For example, dunce and snapbox. We need a non-trivial amount of work to migrate from one to another.

If there is an alternative to me using dunce, I'm willing to change to that and remove the dependency. Removing it without a replacement though will cause covered test cases to fail.

As for snapbox, I think the value-add is worthwhile based on how its helped identify problems we had missed with previous testing strategies and how its sped up updating tests. I intended to explore using snapbox in cargo's tests after this and have explicitly listed it in the deferred items.

Use implicit format-arg capture when possible

Definitely agree for newly written code but almost all of this is written before that was stablized (plus it'll be a hard habit to break until all of my crate's MSRV is new enough...). If its ok, I'll only focus do this on the side and not make it a focus of my changes.

I would try to avoid making items/fields public unless they get value to do so.

I generally agree. Some of the cases were to avoid over-complicating things particularly when dealing with "record" types (data carrying types as compared to abstracted data structures like Vec). In these cases, my preference was to do pub all or nothing.

I feel like some structs do not need to derive Clone.

For the cases of Clone in my mind, I disagree with this. Only implementing fundamental types when they are used, rather than on how they are focusing on how the types are intended to be used, can easily lead to unnecessary code and review churn as they get added/removed based on the current implementation.

[epage]

All comments I've marked as resolved at this point in time were handled in killercup/cargo-edit#670. I've just synced those changes back over and force-pushed.

This style of development is an experiment in seeing what works for importing a command. Normally for a PR I'd do fixups to commits within the PR but since the main commits are meant to mirror specific commits from the cargo-edit repo, I do not want to touch those. Rather than applying feedback in commits on top of those, I've taken this approach.

[epage]

Just pushed a sync of killercup/cargo-edit#671

[weihanglo]

Sorry about flooding lots of comments again. I think we are getting closer to merging!
I'll take a look at most of the test cases soon.

---

BTW I also found a weird case:

x = { package = "tokio", version = "1" }
# cargo add rand --rename x --optional
x = { package = "tokio", version = "1", optional = true }

It looks confusing to me not adding rand crate but I am not sure what the expected behaviour is?

[weihanglo]

Since package.version is mandatory. Is there any use case that people add path dependency but don't want to add it's version? Below is current behaviour:

mylib = { path = "mylib" }
# cargo add ./mylib --optional
mylib = { version = "0.1.0", path = "mylib", optional = true }

And this is what I am considering:

mylib = { path = "mylib" }
# cargo add ./mylib --optional
mylib = { path = "mylib", optional = true }

[epage]

We could preserve the lack of version but it'll be some extra complication that, at the moment, I'm unsure if its worth. When we merge the existing dep with the CLI dep, we preserve the CLI dep's source to make sure we get the version the user specified. We'd need to specialize this based on CLI args.

    let mut dependency = if let Some(mut old_dep) = old_dep.clone() {
        if spec_dep.source().is_some() {
            // Overwrite with `crate_spec`
            old_dep.source = spec_dep.source;
        }
        old_dep = populate_dependency(old_dep, arg);
        old_dep
    } else {
        spec_dep
    };

We do automatically strip versions when using a dev-dependency. I've considered doing the same for crates with publish = false.

[weihanglo]

Actually, this is also an edge case and there is no harm in adding the version back.
(unless it is used to prevent accidental publishing but users should definitely use publish = false instead 😂)

Well, I'm sold. Let's skip it for now.

[epage]

Just so this gets recorded, benefits for not including version:

• In dev-dependencies, skipping version makes it easier for tools like cargo-release to publish cyclic dependencies
• In any dependency, skipping version reduces changes needed when bumping versions, automatic or manual (e.g. I have a workspace with some "cargo xtasks" that aren't published mixed with crates that are but all dependency versions have to be bumped).

[epage]

I just pushed killercup/cargo-edit#672 which has everything that I marked as resolved in the most recent batch of feedback.

[weihanglo]

Two snapshots seem not used anymore:

• cargo_config_source_empty
• git_external

[weihanglo]

Why only optional is preserved?

[epage]

This is when we switch

some-package = { package = "my-package1", version = "0.1.1", optional = true }

to

some-package = { package = "my-package2", version = "99999.0.0", optional = true }

My assumption is that the underlying crate we re-targeted to might have different features, version, registry, etc. The only thing that remains the same is how its used within the current crate, just the dependency key and optional.

[weihanglo]

Actually, I am not sure about the whole thing of overwriting an existing renamed dep with the other package. There is no clue what's going on in stderr. Do you think it is worth a warning or something else?

[weihanglo]

So far the integration looks good to me. Highlight some good parts here again:

• The whole cargo-add command is self-contained. There is nearly no interaction with other existing parts. it's very pleasant to walk through the PR even with six thousand lines of addition.
• Snapshot tests with snapbox: This really improves the overall experience for both developers and reviewers. There are still some missing features in snapbox that we should figure out, but I don't think it's a blocker.
• The development flow of importing an external command, which @epage mentioned here, works well for me. This can be a good reference for future integrations.

Besides what has been mentioned in the PR description, there is an item that needs to tackle in the follow-up:

• Manpage of cargo-add (triggered via cargo help add)

And two unrelated enhancements for cargo internals:

• The road to snapbox: Maybe we can have a guide to porting tests and call out for help?
• Continue fixing messy path issues rooted in cargo with dunce and pathdiff?

Anyway, I guess it's time to see if we can reach a consensus.

[weihanglo]

@rfcbot fcp merge

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• insta-stable resolved by feat: Import cargo-add into cargo #10472 (comment)

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.

[epage]

Just wanted to highlight that approving cargo add is also approving:

• 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

[epage]

As an aside

The whole cargo-add command is self-contained. There is nearly no interaction with other existing parts. it's very pleasant to walk through the PR even with six thousand lines of addition.
...
The development flow of importing an external command, #10472 (comment), works well for me. This can be a good reference for future integrations.

I wonder if another way of modularizing cargo is to take a "porcelain" and "plumbing" model and have the cargo binary focus on "plumbing" where possible and have most commands external, like we have cargo fmt and cargo clippy as external today.

That'd also get us to dog food the command-line API.

[Eh2406]

@rfcbot concern insta-stable

We don't want to make this unstable and make it hard for people to use their existing cargo add.
But this is a very big change to get no unstable time.
Discussion happening at: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add

[weihanglo]

I'm a bit concerned about using symlinks in the test directory. On Windows, it requires developer mode and core.symlinks enabled. I pinged on #t-infra on Zulip to see if anyone had any thoughts. This might cause some problems for people wanting to work on cargo. I think at a minimum there should be some documentation at https://doc.crates.io/contrib/process/working-on-cargo.html#building-cargo that points to the microsoft docs on enabling developer mode and checking core.symlinks.

This has been tested on Windows without symlink support; snapbox will read the files git puts in place of symlinks.

@epage, does this still hold today? I hit this error today on windows-msvc machine. By setting core.symlinks = true globally and re-cloning the repo the issue was fixed.

[epage]

It should. Would love details on a reproduction case.

[weihanglo]

For the symlink error, here are steps to reproduce:

• Create a Windows Server 2022 on AWS EC2 (also failed on my own Windows 11 developer preview)
• Install C++ build tools
• Install Git for Windows (with symlink feature disabled)
• clone rust-lang/cargo
• cargo t --test testsuite cargo_add::add_basic

Feel free to copy this to snapbox issue. Thank you!

running 1 test
test cargo_add::add_basic::case ... FAILED

failures:


---- cargo_add::add_basic::case stdout ----
thread 'cargo_add::add_basic::case' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: "Failed to copy C:\\Users\\Administrator\\Documents\\cargo\\tests\\testsuite\\cargo_add\\add_basic\\in to C:\\Users\\Administrator\\Documents\\cargo\\target\\tmp\\cit\\t0\\case\\: The system cannot find the path specified. (os error 3)", backtrace: None }', C:\Users\Administrator\Documents\cargo\crates\cargo-test-support\src\lib.rs:324:77
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

PS C:\Users\Administrator\Documents\cargo> ls .\tests\testsuite\cargo_add\add_basic\

    Directory: C:\Users\Administrator\Documents\cargo\tests\testsuite\cargo_add\add_basic

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----          3/8/2023   8:18 PM                out
-a----          3/8/2023   8:18 PM             15 in
-a----          3/8/2023   8:18 PM            744 mod.rs
-a----          3/8/2023   8:18 PM             90 stderr.log
-a----          3/8/2023   8:18 PM              0 stdout.log

PS C:\Users\Administrator\Documents\cargo> cat .\tests\testsuite\cargo_add\add_basic\in
../add-basic.in

cargo 1.67.1 (8ecd4f20a 2023-01-10)
release: 1.67.1
commit-hash: 8ecd4f20a9efb626975ac18a016d480dc7183d9b
commit-date: 2023-01-10
host: x86_64-pc-windows-msvc
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.86.0-DEV (sys:0.4.59+curl-7.86.0 vendored ssl:Schannel)
os: Windows 10.0.20348 (Windows Server 2022 Datacenter) [64-bit]