Skip to content

Commit 6a4d98d

Browse files
committed
Auto merge of #10472 - epage:add, r=ehuss
feat: Import cargo-add into cargo ### 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) e.g. - killercup/cargo-edit#521 - killercup/cargo-edit#126 - 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 <details> ```console $ 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 ``` </details> Example commands ```rust 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](https://github.com/killercup/cargo-edit/blob/merge-add/crates/cargo-add/tests/testsuite/cargo_add.rs) 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](https://python-poetry.org/docs/cli/#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](https://yarnpkg.com/cli/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](https://pnpm.io/cli/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](https://go.dev/ref/mod#go-get) - Specify version with ``@<version>`` - Remove dependency with ``@none`` - *(Haskell)* stack doesn't seem to have a native solution - *(Julia)* [pkg Add](https://docs.julialang.org/en/v1/stdlib/Pkg/) - *(Ruby)* [bundle add](https://bundler.io/v2.2/man/bundle-add.1.html) - 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](https://dart.dev/tools/pub/cmd/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](killercup/cargo-edit#590) - 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](https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024). Fixes #5586
2 parents b75281b + a970bfc commit 6a4d98d

File tree

500 files changed

+6546
-1
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

500 files changed

+6546
-1
lines changed

Cargo.toml

+3-1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ glob = "0.3.0"
3535
hex = "0.4"
3636
home = "0.5"
3737
humantime = "2.0.0"
38+
indexmap = "1"
3839
ignore = "0.4.7"
3940
lazy_static = "1.2.0"
4041
jobserver = "0.1.24"
@@ -45,7 +46,7 @@ libgit2-sys = "0.13.2"
4546
memchr = "2.1.3"
4647
opener = "0.5"
4748
os_info = "3.0.7"
48-
pathdiff = "0.2.1"
49+
pathdiff = "0.2"
4950
percent-encoding = "2.0"
5051
rustfix = "0.6.0"
5152
semver = { version = "1.0.3", features = ["serde"] }
@@ -99,6 +100,7 @@ features = [
99100
[dev-dependencies]
100101
cargo-test-macro = { path = "crates/cargo-test-macro" }
101102
cargo-test-support = { path = "crates/cargo-test-support" }
103+
snapbox = { version = "0.2.8", features = ["diff", "path"] }
102104

103105
[build-dependencies]
104106
flate2 = { version = "1.0.3", default-features = false, features = ["zlib"] }

src/bin/cargo/commands/add.rs

+360
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,360 @@
1+
use indexmap::IndexMap;
2+
use indexmap::IndexSet;
3+
4+
use cargo::core::dependency::DepKind;
5+
use cargo::core::FeatureValue;
6+
use cargo::ops::cargo_add::add;
7+
use cargo::ops::cargo_add::AddOptions;
8+
use cargo::ops::cargo_add::DepOp;
9+
use cargo::ops::cargo_add::DepTable;
10+
use cargo::util::command_prelude::*;
11+
use cargo::util::interning::InternedString;
12+
use cargo::CargoResult;
13+
14+
pub fn cli() -> clap::Command<'static> {
15+
clap::Command::new("add")
16+
.setting(clap::AppSettings::DeriveDisplayOrder)
17+
.about("Add dependencies to a Cargo.toml manifest file")
18+
.override_usage(
19+
"\
20+
cargo add [OPTIONS] <DEP>[@<VERSION>] ...
21+
cargo add [OPTIONS] --path <PATH> ...
22+
cargo add [OPTIONS] --git <URL> ..."
23+
)
24+
.after_help("Run `cargo help add` for more detailed information.\n")
25+
.group(clap::ArgGroup::new("selected").multiple(true).required(true))
26+
.args([
27+
clap::Arg::new("crates")
28+
.takes_value(true)
29+
.value_name("DEP_ID")
30+
.multiple_occurrences(true)
31+
.help("Reference to a package to add as a dependency")
32+
.long_help(
33+
"Reference to a package to add as a dependency
34+
35+
You can reference a package by:
36+
- `<name>`, like `cargo add serde` (latest version will be used)
37+
- `<name>@<version-req>`, like `cargo add serde@1` or `cargo add serde@=1.0.38`"
38+
)
39+
.group("selected"),
40+
clap::Arg::new("no-default-features")
41+
.long("no-default-features")
42+
.help("Disable the default features"),
43+
clap::Arg::new("default-features")
44+
.long("default-features")
45+
.help("Re-enable the default features")
46+
.overrides_with("no-default-features"),
47+
clap::Arg::new("features")
48+
.short('F')
49+
.long("features")
50+
.takes_value(true)
51+
.value_name("FEATURES")
52+
.multiple_occurrences(true)
53+
.help("Space or comma separated list of features to activate"),
54+
clap::Arg::new("optional")
55+
.long("optional")
56+
.help("Mark the dependency as optional")
57+
.long_help("Mark the dependency as optional
58+
59+
The package name will be exposed as feature of your crate.")
60+
.conflicts_with("dev"),
61+
clap::Arg::new("no-optional")
62+
.long("no-optional")
63+
.help("Mark the dependency as required")
64+
.long_help("Mark the dependency as required
65+
66+
The package will be removed from your features.")
67+
.conflicts_with("dev")
68+
.overrides_with("optional"),
69+
clap::Arg::new("rename")
70+
.long("rename")
71+
.takes_value(true)
72+
.value_name("NAME")
73+
.help("Rename the dependency")
74+
.long_help("Rename the dependency
75+
76+
Example uses:
77+
- Depending on multiple versions of a crate
78+
- Depend on crates with the same name from different registries"),
79+
])
80+
.arg_manifest_path()
81+
.args([
82+
clap::Arg::new("package")
83+
.short('p')
84+
.long("package")
85+
.takes_value(true)
86+
.value_name("SPEC")
87+
.help("Package to modify"),
88+
clap::Arg::new("offline")
89+
.long("offline")
90+
.help("Run without accessing the network")
91+
])
92+
.arg_quiet()
93+
.arg_dry_run("Don't actually write the manifest")
94+
.next_help_heading("SOURCE")
95+
.args([
96+
clap::Arg::new("path")
97+
.long("path")
98+
.takes_value(true)
99+
.value_name("PATH")
100+
.help("Filesystem path to local crate to add")
101+
.group("selected")
102+
.conflicts_with("git"),
103+
clap::Arg::new("git")
104+
.long("git")
105+
.takes_value(true)
106+
.value_name("URI")
107+
.help("Git repository location")
108+
.long_help("Git repository location
109+
110+
Without any other information, cargo will use latest commit on the main branch.")
111+
.group("selected"),
112+
clap::Arg::new("branch")
113+
.long("branch")
114+
.takes_value(true)
115+
.value_name("BRANCH")
116+
.help("Git branch to download the crate from")
117+
.requires("git")
118+
.group("git-ref"),
119+
clap::Arg::new("tag")
120+
.long("tag")
121+
.takes_value(true)
122+
.value_name("TAG")
123+
.help("Git tag to download the crate from")
124+
.requires("git")
125+
.group("git-ref"),
126+
clap::Arg::new("rev")
127+
.long("rev")
128+
.takes_value(true)
129+
.value_name("REV")
130+
.help("Git reference to download the crate from")
131+
.long_help("Git reference to download the crate from
132+
133+
This is the catch all, handling hashes to named references in remote repositories.")
134+
.requires("git")
135+
.group("git-ref"),
136+
clap::Arg::new("registry")
137+
.long("registry")
138+
.takes_value(true)
139+
.value_name("NAME")
140+
.help("Package registry for this dependency"),
141+
])
142+
.next_help_heading("SECTION")
143+
.args([
144+
clap::Arg::new("dev")
145+
.long("dev")
146+
.help("Add as development dependency")
147+
.long_help("Add as development dependency
148+
149+
Dev-dependencies are not used when compiling a package for building, but are used for compiling tests, examples, and benchmarks.
150+
151+
These dependencies are not propagated to other packages which depend on this package.")
152+
.group("section"),
153+
clap::Arg::new("build")
154+
.long("build")
155+
.help("Add as build dependency")
156+
.long_help("Add as build dependency
157+
158+
Build-dependencies are the only dependencies available for use by build scripts (`build.rs` files).")
159+
.group("section"),
160+
clap::Arg::new("target")
161+
.long("target")
162+
.takes_value(true)
163+
.value_name("TARGET")
164+
.forbid_empty_values(true)
165+
.help("Add as dependency to the given target platform")
166+
])
167+
}
168+
169+
pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
170+
let dry_run = args.is_present("dry-run");
171+
let section = parse_section(args);
172+
173+
let ws = args.workspace(config)?;
174+
let packages = args.packages_from_flags()?;
175+
let packages = packages.get_packages(&ws)?;
176+
let spec = match packages.len() {
177+
0 => {
178+
return Err(CliError::new(
179+
anyhow::format_err!("no packages selected. Please specify one with `-p <PKGID>`"),
180+
101,
181+
));
182+
}
183+
1 => packages[0],
184+
len => {
185+
return Err(CliError::new(
186+
anyhow::format_err!(
187+
"{len} packages selected. Please specify one with `-p <PKGID>`",
188+
),
189+
101,
190+
));
191+
}
192+
};
193+
194+
let dependencies = parse_dependencies(config, args)?;
195+
196+
let options = AddOptions {
197+
config,
198+
spec,
199+
dependencies,
200+
section,
201+
dry_run,
202+
};
203+
add(&ws, &options)?;
204+
205+
Ok(())
206+
}
207+
208+
fn parse_dependencies(config: &Config, matches: &ArgMatches) -> CargoResult<Vec<DepOp>> {
209+
let path = matches.value_of("path");
210+
let git = matches.value_of("git");
211+
let branch = matches.value_of("branch");
212+
let rev = matches.value_of("rev");
213+
let tag = matches.value_of("tag");
214+
let rename = matches.value_of("rename");
215+
let registry = matches.registry(config)?;
216+
let default_features = default_features(matches);
217+
let optional = optional(matches);
218+
219+
let mut crates = matches
220+
.values_of("crates")
221+
.into_iter()
222+
.flatten()
223+
.map(|c| (Some(String::from(c)), None))
224+
.collect::<IndexMap<_, _>>();
225+
let mut infer_crate_name = false;
226+
if crates.is_empty() {
227+
if path.is_some() || git.is_some() {
228+
crates.insert(None, None);
229+
infer_crate_name = true;
230+
} else {
231+
unreachable!("clap should ensure we have some source selected");
232+
}
233+
}
234+
for feature in matches
235+
.values_of("features")
236+
.into_iter()
237+
.flatten()
238+
.flat_map(parse_feature)
239+
{
240+
let parsed_value = FeatureValue::new(InternedString::new(feature));
241+
match parsed_value {
242+
FeatureValue::Feature(_) => {
243+
if 1 < crates.len() {
244+
let candidates = crates
245+
.keys()
246+
.map(|c| {
247+
format!(
248+
"`{}/{}`",
249+
c.as_deref().expect("only none when there is 1"),
250+
feature
251+
)
252+
})
253+
.collect::<Vec<_>>();
254+
anyhow::bail!("feature `{feature}` must be qualified by the dependency its being activated for, like {}", candidates.join(", "));
255+
}
256+
crates
257+
.first_mut()
258+
.expect("always at least one crate")
259+
.1
260+
.get_or_insert_with(IndexSet::new)
261+
.insert(feature.to_owned());
262+
}
263+
FeatureValue::Dep { .. } => {
264+
anyhow::bail!("feature `{feature}` is not allowed to use explicit `dep:` syntax",)
265+
}
266+
FeatureValue::DepFeature {
267+
dep_name,
268+
dep_feature,
269+
..
270+
} => {
271+
if infer_crate_name {
272+
anyhow::bail!("`{feature}` is unsupported when inferring the crate name, use `{dep_feature}`");
273+
}
274+
if dep_feature.contains('/') {
275+
anyhow::bail!("multiple slashes in feature `{feature}` is not allowed");
276+
}
277+
crates.get_mut(&Some(dep_name.as_str().to_owned())).ok_or_else(|| {
278+
anyhow::format_err!("feature `{dep_feature}` activated for crate `{dep_name}` but the crate wasn't specified")
279+
})?
280+
.get_or_insert_with(IndexSet::new)
281+
.insert(dep_feature.as_str().to_owned());
282+
}
283+
}
284+
}
285+
286+
let mut deps: Vec<DepOp> = Vec::new();
287+
for (crate_spec, features) in crates {
288+
let dep = DepOp {
289+
crate_spec,
290+
rename: rename.map(String::from),
291+
features,
292+
default_features,
293+
optional,
294+
registry: registry.clone(),
295+
path: path.map(String::from),
296+
git: git.map(String::from),
297+
branch: branch.map(String::from),
298+
rev: rev.map(String::from),
299+
tag: tag.map(String::from),
300+
};
301+
deps.push(dep);
302+
}
303+
304+
if deps.len() > 1 && rename.is_some() {
305+
anyhow::bail!("cannot specify multiple crates with `--rename`");
306+
}
307+
308+
Ok(deps)
309+
}
310+
311+
fn default_features(matches: &ArgMatches) -> Option<bool> {
312+
resolve_bool_arg(
313+
matches.is_present("default-features"),
314+
matches.is_present("no-default-features"),
315+
)
316+
}
317+
318+
fn optional(matches: &ArgMatches) -> Option<bool> {
319+
resolve_bool_arg(
320+
matches.is_present("optional"),
321+
matches.is_present("no-optional"),
322+
)
323+
}
324+
325+
fn resolve_bool_arg(yes: bool, no: bool) -> Option<bool> {
326+
match (yes, no) {
327+
(true, false) => Some(true),
328+
(false, true) => Some(false),
329+
(false, false) => None,
330+
(_, _) => unreachable!("clap should make this impossible"),
331+
}
332+
}
333+
334+
fn parse_section(matches: &ArgMatches) -> DepTable {
335+
let kind = if matches.is_present("dev") {
336+
DepKind::Development
337+
} else if matches.is_present("build") {
338+
DepKind::Build
339+
} else {
340+
DepKind::Normal
341+
};
342+
343+
let mut table = DepTable::new().set_kind(kind);
344+
345+
if let Some(target) = matches.value_of("target") {
346+
assert!(!target.is_empty(), "Target specification may not be empty");
347+
table = table.set_target(target);
348+
}
349+
350+
table
351+
}
352+
353+
/// Split feature flag list
354+
fn parse_feature(feature: &str) -> impl Iterator<Item = &str> {
355+
// Not re-using `CliFeatures` because it uses a BTreeSet and loses user's ordering
356+
feature
357+
.split_whitespace()
358+
.flat_map(|s| s.split(','))
359+
.filter(|s| !s.is_empty())
360+
}

0 commit comments

Comments
 (0)