From 772e1a17b4e528c602d48002593726ebf6d126cf Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 28 Nov 2016 08:51:36 -0800 Subject: [PATCH] Test for bad path overrides with summaries Bad path overrides are currently detected to issue warnings in cases where path overrides are not suitable and have exhibited buggy behavior in the past. Unfortunately though it looks like some false positives are being issued, causing unnecessary confusion about `paths` overrides. This commit fixes the detection of these "bad path overrides" by comparing `Summary` dependencies (what's written down in `Cargo.toml`) rather than comparing the `Cargo.toml` of the override with `Cargo.lock`. We're guaranteed that the package we're overridding has already been resolved into `Cargo.lock`, so we know that if the two `Cargo.toml` files are equivalent we'll continue with the same crate graph. I'm not actually entirely sure why I originally thought it'd be better to go through the `Cargo.lock` comparison route. Unfortunately that doesn't take into account optional deps which aren't in `Cargo.lock` but are in `Cargo.toml` of the override, causing the false positive. This method, however, simply ensures that the two dependency lists are the same. Closes #3313 --- src/cargo/core/dependency.rs | 2 +- src/cargo/core/registry.rs | 20 +------- tests/overrides.rs | 90 ++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index 23cb71034b4..377f21dee06 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -11,7 +11,7 @@ use util::{CargoError, CargoResult, Cfg, CfgExpr, ChainError, human, Config}; /// Information about a dependency requested by a Cargo manifest. /// Cheap to copy. -#[derive(PartialEq, Clone ,Debug)] +#[derive(PartialEq, Clone, Debug)] pub struct Dependency { inner: Rc, } diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index a798a49a504..ec458f8fb9c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -290,23 +290,7 @@ impl<'cfg> PackageRegistry<'cfg> { fn warn_bad_override(&self, override_summary: &Summary, real_summary: &Summary) -> CargoResult<()> { - let real = real_summary.package_id(); - // If we don't have a locked variant then things are going quite wrong - // at this point, but we've already emitted a warning, so don't worry - // about it. - let map = match self.locked.get(real.source_id()) { - Some(map) => map, - None => return Ok(()), - }; - let list = map.get(real.name()).chain_error(|| { - human(format!("failed to find lock name of {}", real)) - })?; - let &(_, ref real_deps) = list.iter().find(|&&(ref id, _)| { - real == id - }).chain_error(|| { - human(format!("failed to find lock version of {}", real)) - })?; - let mut real_deps = real_deps.clone(); + let mut real_deps = real_summary.dependencies().iter().collect::>(); let boilerplate = "\ This is currently allowed but is known to produce buggy behavior with spurious @@ -322,7 +306,7 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies "; for dep in override_summary.dependencies() { - if let Some(i) = real_deps.iter().position(|id| dep.matches_id(id)) { + if let Some(i) = real_deps.iter().position(|d| dep == *d) { real_deps.remove(i); continue } diff --git a/tests/overrides.rs b/tests/overrides.rs index 2ba54fbc271..d92f81791cc 100644 --- a/tests/overrides.rs +++ b/tests/overrides.rs @@ -1013,3 +1013,93 @@ fn replace_to_path_dep() { assert_that(p.cargo_process("build"), execs().with_status(0)); } + +#[test] +fn paths_ok_with_optional() { + Package::new("bar", "0.1.0").publish(); + + let p = project("local") + .file("Cargo.toml", r#" + [package] + name = "local" + version = "0.0.1" + authors = [] + + [dependencies] + foo = { path = "foo" } + "#) + .file("src/lib.rs", "") + .file("foo/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [dependencies] + bar = { version = "0.1", optional = true } + "#) + .file("foo/src/lib.rs", "") + .file("foo2/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [dependencies] + bar = { version = "0.1", optional = true } + "#) + .file("foo2/src/lib.rs", "") + .file(".cargo/config", r#" + paths = ["foo2"] + "#); + + assert_that(p.cargo_process("build"), + execs().with_status(0).with_stderr("\ +[COMPILING] foo v0.1.0 ([..]foo2) +[COMPILING] local v0.0.1 ([..]) +[FINISHED] [..] +")); +} + +#[test] +fn paths_add_optional_bad() { + Package::new("bar", "0.1.0").publish(); + + let p = project("local") + .file("Cargo.toml", r#" + [package] + name = "local" + version = "0.0.1" + authors = [] + + [dependencies] + foo = { path = "foo" } + "#) + .file("src/lib.rs", "") + .file("foo/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("foo/src/lib.rs", "") + .file("foo2/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [dependencies] + bar = { version = "0.1", optional = true } + "#) + .file("foo2/src/lib.rs", "") + .file(".cargo/config", r#" + paths = ["foo2"] + "#); + + assert_that(p.cargo_process("build"), + execs().with_status(0).with_stderr_contains("\ +warning: path override for crate `foo` has altered the original list of +dependencies; the dependency on `bar` was either added or\ +")); +}