Skip to content

Commit

Permalink
Auto merge of #10650 - epage:install, r=ehuss
Browse files Browse the repository at this point in the history
feat(install): Support `foo@version` like cargo-add

### What does this PR try to resolve?

This aims to make `cargo install` consistent with
- `cargo add foo@version` from #10472
- pkgid changes in #10582
- `cargo yank foo@version` from #10597

It also offers a shorthand for people installing a specific version.

### How should we test and review this PR?

#10582 acted as the FCP for this, see #10597

Documentation updates are split into their own commit to not clog up browsing the code.

Examine the tests to see if they make sense

### Additional information

While the `foo@vewrsion` syntax is the same, each's semantics are different.  We had decided it was better to have the same syntax with different semantics than having the user worry about what syntax they use where.  In `cargo install`s case, it has an
implicit-but-required `=` operand while `cargo-add` allows any operand.

This doesn't use the full `pkgid` syntax because that allows syntax that
is unsupported here.

This doesn't use `cargo-add`s parser because that is for version reqs.

I held off on reusing the parser from `cargo-yank` because they had
different type system needs and the level of duplication didn't seem
worth it (see Rule of Three).
  • Loading branch information
bors committed May 11, 2022
2 parents 08bef9d + f063c65 commit 23ae8a0
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 22 deletions.
24 changes: 21 additions & 3 deletions src/bin/cargo/commands/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
// but not `Config::reload_rooted_at` which is always cwd)
let path = path.map(|p| paths::normalize_path(&p));

let version = args.value_of("version");
let krates = args
.values_of("crate")
.unwrap_or_default()
.collect::<Vec<_>>();
.map(|k| resolve_crate(k, version))
.collect::<crate::CargoResult<Vec<_>>>()?;

let mut from_cwd = false;

Expand Down Expand Up @@ -129,7 +131,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
SourceId::crates_io(config)?
};

let version = args.value_of("version");
let root = args.value_of("root");

// We only provide workspace information for local crate installation from
Expand Down Expand Up @@ -166,11 +167,28 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
krates,
source,
from_cwd,
version,
&compile_opts,
args.is_present("force"),
args.is_present("no-track"),
)?;
}
Ok(())
}

fn resolve_crate<'k>(
mut krate: &'k str,
mut version: Option<&'k str>,
) -> crate::CargoResult<(&'k str, Option<&'k str>)> {
if let Some((k, v)) = krate.split_once('@') {
if version.is_some() {
anyhow::bail!("cannot specify both `@{v}` and `--version`");
}
if k.is_empty() {
// by convention, arguments starting with `@` are response files
anyhow::bail!("missing crate name for `@{v}`");
}
krate = k;
version = Some(v);
}
Ok((krate, version))
}
22 changes: 8 additions & 14 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -556,10 +556,9 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
pub fn install(
config: &Config,
root: Option<&str>,
krates: Vec<&str>,
krates: Vec<(&str, Option<&str>)>,
source_id: SourceId,
from_cwd: bool,
vers: Option<&str>,
opts: &ops::CompileOptions,
force: bool,
no_track: bool,
Expand All @@ -569,18 +568,13 @@ pub fn install(
let map = SourceConfigMap::new(config)?;

let (installed_anything, scheduled_error) = if krates.len() <= 1 {
let (krate, vers) = krates
.into_iter()
.next()
.map(|(k, v)| (Some(k), v))
.unwrap_or((None, None));
let installable_pkg = InstallablePackage::new(
config,
root,
map,
krates.into_iter().next(),
source_id,
from_cwd,
vers,
opts,
force,
no_track,
true,
config, root, map, krate, source_id, from_cwd, vers, opts, force, no_track, true,
)?;
let mut installed_anything = true;
if let Some(installable_pkg) = installable_pkg {
Expand All @@ -596,7 +590,7 @@ pub fn install(

let pkgs_to_install: Vec<_> = krates
.into_iter()
.filter_map(|krate| {
.filter_map(|(krate, vers)| {
let root = root.clone();
let map = map.clone();
match InstallablePackage::new(
Expand Down
2 changes: 1 addition & 1 deletion src/doc/man/cargo-install.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ cargo-install - Build and install a Rust binary

## SYNOPSIS

`cargo install` [_options_] _crate_...\
`cargo install` [_options_] _crate_[@_version_]...\
`cargo install` [_options_] `--path` _path_\
`cargo install` [_options_] `--git` _url_ [_crate_...]\
`cargo install` [_options_] `--list`
Expand Down
2 changes: 1 addition & 1 deletion src/doc/man/generated_txt/cargo-install.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ NAME
cargo-install - Build and install a Rust binary

SYNOPSIS
cargo install [options] crate...
cargo install [options] crate[@version]...
cargo install [options] --path path
cargo install [options] --git url [crate...]
cargo install [options] --list
Expand Down
2 changes: 1 addition & 1 deletion src/doc/src/commands/cargo-install.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ cargo-install - Build and install a Rust binary

## SYNOPSIS

`cargo install` [_options_] _crate_...\
`cargo install` [_options_] _crate_[@_version_]...\
`cargo install` [_options_] `--path` _path_\
`cargo install` [_options_] `--git` _url_ [_crate_...]\
`cargo install` [_options_] `--list`
Expand Down
2 changes: 1 addition & 1 deletion src/etc/man/cargo-install.1
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
.SH "NAME"
cargo\-install \- Build and install a Rust binary
.SH "SYNOPSIS"
\fBcargo install\fR [\fIoptions\fR] \fIcrate\fR\&...
\fBcargo install\fR [\fIoptions\fR] \fIcrate\fR[@\fIversion\fR]\&...
.br
\fBcargo install\fR [\fIoptions\fR] \fB\-\-path\fR \fIpath\fR
.br
Expand Down
49 changes: 48 additions & 1 deletion tests/testsuite/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ fn vers_precise() {
}

#[cargo_test]
fn version_too() {
fn version_precise() {
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");

Expand All @@ -1391,6 +1391,53 @@ fn version_too() {
.run();
}

#[cargo_test]
fn inline_version_precise() {
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");

cargo_process("install foo@0.1.1")
.with_stderr_contains("[DOWNLOADED] foo v0.1.1 (registry [..])")
.run();
}

#[cargo_test]
fn inline_version_multiple() {
pkg("foo", "0.1.0");
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");
pkg("bar", "0.2.0");
pkg("bar", "0.2.1");
pkg("bar", "0.2.2");

cargo_process("install foo@0.1.1 bar@0.2.1")
.with_stderr_contains("[DOWNLOADED] foo v0.1.1 (registry [..])")
.with_stderr_contains("[DOWNLOADED] bar v0.2.1 (registry [..])")
.run();
}

#[cargo_test]
fn inline_version_without_name() {
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");

cargo_process("install @0.1.1")
.with_status(101)
.with_stderr("error: missing crate name for `@0.1.1`")
.run();
}

#[cargo_test]
fn inline_and_explicit_version() {
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");

cargo_process("install foo@0.1.1 --version 0.1.1")
.with_status(101)
.with_stderr("error: cannot specify both `@0.1.1` and `--version`")
.run();
}

#[cargo_test]
fn not_both_vers_and_version() {
pkg("foo", "0.1.1");
Expand Down

0 comments on commit 23ae8a0

Please sign in to comment.