Skip to content

Commit

Permalink
Auto merge of #3338 - alexcrichton:correct-vers-flag, r=brson
Browse files Browse the repository at this point in the history
Require `cargo install --vers` takes a semver version

Historically Cargo accidentally took a semver version *requirement*, so let's
start issuing warnings about how this is now legacy behavior.

Closes #3321
  • Loading branch information
bors committed Dec 2, 2016
2 parents 307a613 + d91c9b4 commit 787591c
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 4 deletions.
28 changes: 25 additions & 3 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::io::prelude::*;
use std::io::SeekFrom;
use std::path::{Path, PathBuf};

use semver::Version;
use tempdir::TempDir;
use toml;

Expand Down Expand Up @@ -57,7 +58,7 @@ pub fn install(root: Option<&str>,
let map = SourceConfigMap::new(config)?;
let (pkg, source) = if source_id.is_git() {
select_pkg(GitSource::new(source_id, config), source_id,
krate, vers, &mut |git| git.read_packages())?
krate, vers, config, &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");
Expand All @@ -68,11 +69,11 @@ pub fn install(root: Option<&str>,
specify an alternate source", path.display()))
})?;
select_pkg(PathSource::new(&path, source_id, config),
source_id, krate, vers,
source_id, krate, vers, config,
&mut |path| path.read_packages())?
} else {
select_pkg(map.load(source_id)?,
source_id, krate, vers,
source_id, krate, vers, config,
&mut |_| Err(human("must specify a crate to install from \
crates.io, or use --path or --git to \
specify alternate source")))?
Expand Down Expand Up @@ -251,13 +252,34 @@ fn select_pkg<'a, T>(mut source: T,
source_id: &SourceId,
name: Option<&str>,
vers: Option<&str>,
config: &Config,
list_all: &mut FnMut(&mut T) -> CargoResult<Vec<Package>>)
-> CargoResult<(Package, Box<Source + 'a>)>
where T: Source + 'a
{
source.update()?;
match name {
Some(name) => {
let vers = match vers {
Some(v) => {
match v.parse::<Version>() {
Ok(v) => Some(format!("={}", v)),
Err(_) => {
let msg = format!("the `--vers` provided, `{}`, is \
not a valid semver version\n\n\
historically Cargo treated this \
as a semver version requirement \
accidentally\nand will continue \
to do so, but this behavior \
will be removed eventually", v);
config.shell().warn(&msg)?;
Some(v.to_string())
}
}
}
None => None,
};
let vers = vers.as_ref().map(|s| &**s);
let dep = Dependency::parse_no_deprecated(name, vers, source_id)?;
let deps = source.query(&dep)?;
match deps.iter().map(|p| p.package_id()).max() {
Expand Down
26 changes: 25 additions & 1 deletion tests/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ fn bad_version() {
assert_that(cargo_process("install").arg("foo").arg("--vers=0.2.0"),
execs().with_status(101).with_stderr("\
[UPDATING] registry [..]
[ERROR] could not find `foo` in `registry [..]` with version `0.2.0`
[ERROR] could not find `foo` in `registry [..]` with version `=0.2.0`
"));
}

Expand Down Expand Up @@ -826,3 +826,27 @@ fn use_path_workspace() {
let lock2 = p.read_lockfile();
assert!(lock == lock2, "different lockfiles");
}

#[test]
fn vers_precise() {
pkg("foo", "0.1.1");
pkg("foo", "0.1.2");

assert_that(cargo_process("install").arg("foo").arg("--vers").arg("0.1.1"),
execs().with_status(0).with_stderr_contains("\
[DOWNLOADING] foo v0.1.1 (registry [..])
"));
}

#[test]
fn legacy_version_requirement() {
pkg("foo", "0.1.1");

assert_that(cargo_process("install").arg("foo").arg("--vers").arg("0.1"),
execs().with_status(0).with_stderr_contains("\
warning: the `--vers` provided, `0.1`, is not a valid semver version
historically Cargo treated this as a semver version requirement accidentally
and will continue to do so, but this behavior will be removed eventually
"));
}

0 comments on commit 787591c

Please sign in to comment.