Skip to content

Commit c99ae7e

Browse files
borsbors
committed
Auto merge of #3336 - alexcrichton:fix-warning, r=brson
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
2 parents c82fd7f + 772e1a1 commit c99ae7e

File tree

3 files changed

+93
-19
lines changed

3 files changed

+93
-19
lines changed

‎src/cargo/core/dependency.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use util::{CargoError, CargoResult, Cfg, CfgExpr, ChainError, human, Config};
1111

1212
/// Information about a dependency requested by a Cargo manifest.
1313
/// Cheap to copy.
14-
#[derive(PartialEq, Clone ,Debug)]
14+
#[derive(PartialEq, Clone, Debug)]
1515
pub struct Dependency {
1616
inner: Rc<DependencyInner>,
1717
}

‎src/cargo/core/registry.rs

+2-18
Original file line numberDiff line numberDiff line change
@@ -290,23 +290,7 @@ impl<'cfg> PackageRegistry<'cfg> {
290290
fn warn_bad_override(&self,
291291
override_summary: &Summary,
292292
real_summary: &Summary) -> CargoResult<()> {
293-
let real = real_summary.package_id();
294-
// If we don't have a locked variant then things are going quite wrong
295-
// at this point, but we've already emitted a warning, so don't worry
296-
// about it.
297-
let map = match self.locked.get(real.source_id()) {
298-
Some(map) => map,
299-
None => return Ok(()),
300-
};
301-
let list = map.get(real.name()).chain_error(|| {
302-
human(format!("failed to find lock name of {}", real))
303-
})?;
304-
let &(_, ref real_deps) = list.iter().find(|&&(ref id, _)| {
305-
real == id
306-
}).chain_error(|| {
307-
human(format!("failed to find lock version of {}", real))
308-
})?;
309-
let mut real_deps = real_deps.clone();
293+
let mut real_deps = real_summary.dependencies().iter().collect::<Vec<_>>();
310294

311295
let boilerplate = "\
312296
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
322306
";
323307

324308
for dep in override_summary.dependencies() {
325-
if let Some(i) = real_deps.iter().position(|id| dep.matches_id(id)) {
309+
if let Some(i) = real_deps.iter().position(|d| dep == *d) {
326310
real_deps.remove(i);
327311
continue
328312
}

‎tests/overrides.rs

+90
Original file line numberDiff line numberDiff line change
@@ -1013,3 +1013,93 @@ fn replace_to_path_dep() {
10131013
assert_that(p.cargo_process("build"),
10141014
execs().with_status(0));
10151015
}
1016+
1017+
#[test]
1018+
fn paths_ok_with_optional() {
1019+
Package::new("bar", "0.1.0").publish();
1020+
1021+
let p = project("local")
1022+
.file("Cargo.toml", r#"
1023+
[package]
1024+
name = "local"
1025+
version = "0.0.1"
1026+
authors = []
1027+
1028+
[dependencies]
1029+
foo = { path = "foo" }
1030+
"#)
1031+
.file("src/lib.rs", "")
1032+
.file("foo/Cargo.toml", r#"
1033+
[package]
1034+
name = "foo"
1035+
version = "0.1.0"
1036+
authors = []
1037+
1038+
[dependencies]
1039+
bar = { version = "0.1", optional = true }
1040+
"#)
1041+
.file("foo/src/lib.rs", "")
1042+
.file("foo2/Cargo.toml", r#"
1043+
[package]
1044+
name = "foo"
1045+
version = "0.1.0"
1046+
authors = []
1047+
1048+
[dependencies]
1049+
bar = { version = "0.1", optional = true }
1050+
"#)
1051+
.file("foo2/src/lib.rs", "")
1052+
.file(".cargo/config", r#"
1053+
paths = ["foo2"]
1054+
"#);
1055+
1056+
assert_that(p.cargo_process("build"),
1057+
execs().with_status(0).with_stderr("\
1058+
[COMPILING] foo v0.1.0 ([..]foo2)
1059+
[COMPILING] local v0.0.1 ([..])
1060+
[FINISHED] [..]
1061+
"));
1062+
}
1063+
1064+
#[test]
1065+
fn paths_add_optional_bad() {
1066+
Package::new("bar", "0.1.0").publish();
1067+
1068+
let p = project("local")
1069+
.file("Cargo.toml", r#"
1070+
[package]
1071+
name = "local"
1072+
version = "0.0.1"
1073+
authors = []
1074+
1075+
[dependencies]
1076+
foo = { path = "foo" }
1077+
"#)
1078+
.file("src/lib.rs", "")
1079+
.file("foo/Cargo.toml", r#"
1080+
[package]
1081+
name = "foo"
1082+
version = "0.1.0"
1083+
authors = []
1084+
"#)
1085+
.file("foo/src/lib.rs", "")
1086+
.file("foo2/Cargo.toml", r#"
1087+
[package]
1088+
name = "foo"
1089+
version = "0.1.0"
1090+
authors = []
1091+
1092+
[dependencies]
1093+
bar = { version = "0.1", optional = true }
1094+
"#)
1095+
.file("foo2/src/lib.rs", "")
1096+
.file(".cargo/config", r#"
1097+
paths = ["foo2"]
1098+
"#);
1099+
1100+
assert_that(p.cargo_process("build"),
1101+
execs().with_status(0).with_stderr_contains("\
1102+
warning: path override for crate `foo` has altered the original list of
1103+
dependencies; the dependency on `bar` was either added or\
1104+
"));
1105+
}

0 commit comments

Comments
 (0)