Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo install multiple crates #4216

Merged
merged 11 commits into from
Jul 28, 2017
10 changes: 5 additions & 5 deletions src/bin/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub struct Options {
flag_frozen: bool,
flag_locked: bool,

arg_crate: Option<String>,
arg_crate: Vec<String>,
flag_vers: Option<String>,

flag_git: Option<String>,
Expand All @@ -37,7 +37,7 @@ pub const USAGE: &'static str = "
Install a Rust binary

Usage:
cargo install [options] [<crate>]
cargo install [options] [<crate>...]
cargo install [options] --list

Specifying what crate to install:
Expand Down Expand Up @@ -139,20 +139,20 @@ pub fn execute(options: Options, config: &Config) -> CliResult {
SourceId::for_git(&url, gitref)
} else if let Some(path) = options.flag_path {
SourceId::for_path(&config.cwd().join(path))?
} else if options.arg_crate == None {
} else if options.arg_crate.is_empty() {
SourceId::for_path(&config.cwd())?
} else {
SourceId::crates_io(config)?
};

let krate = options.arg_crate.as_ref().map(|s| &s[..]);
let krates = options.arg_crate.iter().map(|s| &s[..]).collect::<Vec<_>>();
let vers = options.flag_vers.as_ref().map(|s| &s[..]);
let root = options.flag_root.as_ref().map(|s| &s[..]);

if options.flag_list {
ops::install_list(root, config)?;
} else {
ops::install(root, krate, &source, vers, &compile_opts, options.flag_force)?;
ops::install(root, krates, &source, vers, &compile_opts, options.flag_force)?;
}
Ok(())
}
63 changes: 53 additions & 10 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ use std::env;
use std::ffi::OsString;
use std::fs::{self, File};
use std::io::prelude::*;
use std::io::SeekFrom;
use std::io::{self, SeekFrom};
use std::path::{Path, PathBuf};
use std::sync::Arc;
use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT, Ordering};

use semver::Version;
use tempdir::TempDir;
Expand Down Expand Up @@ -55,37 +56,75 @@ impl Drop for Transaction {
}

pub fn install(root: Option<&str>,
krates: Vec<&str>,
source_id: &SourceId,
vers: Option<&str>,
opts: &ops::CompileOptions,
force: bool) -> CargoResult<()> {
let root = resolve_root(root, opts.config)?;
let map = SourceConfigMap::new(opts.config)?;

if krates.len() <= 1 {
install_one(root, map, krates.into_iter().next(), source_id, vers, opts, force)
} else {
let mut success = vec![];
let mut errors = vec![];
for krate in krates {
let root = root.clone();
let map = map.clone();
match install_one(root, map, Some(krate), source_id, vers, opts, force) {
Ok(()) => success.push(krate),
Err(e) => errors.push(format!("{}: {}", krate, e))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got standard error handling mechanisms which print contextual information, and this loses the error backtrace. Can this just be propagated outwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I didn't want errors to be lost in a sea of command-line output, so I thought it'd be better to collect them at the end.

}
}

writeln!(io::stderr(),
"\n\nSUMMARY\n\nSuccessfully installed: {}\n\nErrors:\n\t{}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use the standard shell methods for reporting what happened here? Also can the whitespace be trimmed down as it seems like it's quite a bit?

success.join(", "),
errors.join("\n\t"))?;

Ok(())
}
}

fn install_one(root: Filesystem,
map: SourceConfigMap,
krate: Option<&str>,
source_id: &SourceId,
vers: Option<&str>,
opts: &ops::CompileOptions,
force: bool) -> CargoResult<()> {

static ALREADY_UPDATED: AtomicBool = ATOMIC_BOOL_INIT;
let needs_update = !ALREADY_UPDATED.load(Ordering::SeqCst);
ALREADY_UPDATED.store(true, Ordering::SeqCst);

let config = opts.config;
let root = resolve_root(root, config)?;
let map = SourceConfigMap::new(config)?;

let (pkg, source) = if source_id.is_git() {
select_pkg(GitSource::new(source_id, config),
krate, vers, config, &mut |git| git.read_packages())?
krate, vers, config, needs_update,
&mut |git| git.read_packages())?
} else if source_id.is_path() {
let path = source_id.url().to_file_path().ok()
.expect("path sources must have a valid path");
let path = source_id.url().to_file_path()
.map_err(|()| CargoError::from("path sources must have a valid path"))?;
let mut src = PathSource::new(&path, source_id, config);
src.update().chain_err(|| {
format!("`{}` is not a crate root; specify a crate to \
install from crates.io, or use --path or --git to \
specify an alternate source", path.display())
})?;
select_pkg(PathSource::new(&path, source_id, config),
krate, vers, config, &mut |path| path.read_packages())?
krate, vers, config, needs_update,
&mut |path| path.read_packages())?
} else {
select_pkg(map.load(source_id)?,
krate, vers, config,
krate, vers, config, needs_update,
&mut |_| Err("must specify a crate to install from \
crates.io, or use --path or --git to \
specify alternate source".into()))?
};


let mut td_opt = None;
let overidden_target_dir = if source_id.is_path() {
None
Expand Down Expand Up @@ -267,11 +306,15 @@ fn select_pkg<'a, T>(mut source: T,
name: Option<&str>,
vers: Option<&str>,
config: &Config,
needs_update: bool,
list_all: &mut FnMut(&mut T) -> CargoResult<Vec<Package>>)
-> CargoResult<(Package, Box<Source + 'a>)>
where T: Source + 'a
{
source.update()?;
if needs_update {
source.update()?;
}

match name {
Some(name) => {
let vers = match vers {
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/sources/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use util::{Config, ToUrl};
use util::config::ConfigValue;
use util::errors::{CargoError, CargoResult, CargoResultExt};

#[derive(Clone)]
pub struct SourceConfigMap<'cfg> {
cfgs: HashMap<String, SourceConfig>,
id2name: HashMap<SourceId, String>,
Expand All @@ -28,6 +29,7 @@ pub struct SourceConfigMap<'cfg> {
/// registry = 'https://github.com/rust-lang/crates.io-index'
/// replace-with = 'foo' # optional
/// ```
#[derive(Clone)]
struct SourceConfig {
// id this source corresponds to, inferred from the various defined keys in
// the configuration
Expand Down
47 changes: 47 additions & 0 deletions tests/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,53 @@ warning: be sure to add `[..]` to your PATH to be able to run the installed bina
assert_that(cargo_home(), is_not(has_installed_exe("foo")));
}

#[test]
fn multiple_pkgs() {
pkg("foo", "0.0.1");
pkg("bar", "0.0.1");

assert_that(cargo_process("install").arg("foo").arg("bar"),
execs().with_status(0).with_stderr(&format!("\
[UPDATING] registry `[..]`
[DOWNLOADING] foo v0.0.1 (registry file://[..])
[INSTALLING] foo v0.0.1
[COMPILING] foo v0.0.1
[FINISHED] release [optimized] target(s) in [..]
[INSTALLING] {home}[..]bin[..]foo[..]
warning: be sure to add `[..]` to your PATH to be able to run the installed binaries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this warning be printed out only once?

[DOWNLOADING] bar v0.0.1 (registry file://[..])
[INSTALLING] bar v0.0.1
[COMPILING] bar v0.0.1
[FINISHED] release [optimized] target(s) in [..]
[INSTALLING] {home}[..]bin[..]bar[..]
warning: be sure to add `[..]` to your PATH to be able to run the installed binaries


SUMMARY

Successfully installed: foo, bar

Errors:
<tab>
",
home = cargo_home().display())));
assert_that(cargo_home(), has_installed_exe("foo"));
assert_that(cargo_home(), has_installed_exe("bar"));

assert_that(cargo_process("uninstall").arg("foo"),
execs().with_status(0).with_stderr(&format!("\
[REMOVING] {home}[..]bin[..]foo[..]
",
home = cargo_home().display())));
assert_that(cargo_process("uninstall").arg("bar"),
execs().with_status(0).with_stderr(&format!("\
[REMOVING] {home}[..]bin[..]bar[..]
",
home = cargo_home().display())));
assert_that(cargo_home(), is_not(has_installed_exe("foo")));
assert_that(cargo_home(), is_not(has_installed_exe("bar")));
}

#[test]
fn pick_max_version() {
pkg("foo", "0.0.1");
Expand Down