Skip to content

Commit

Permalink
Avoid visiting deps too often in resolve
Browse files Browse the repository at this point in the history
If we're activating an already-active version of a dependency, then there's no
need to actually recurse into it. Instead, we can skip it if we're not enabling
any extra features in it to avoid extraneous recursion.
  • Loading branch information
alexcrichton committed Oct 22, 2014
1 parent 82e469e commit 7db89ed
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 29 deletions.
50 changes: 31 additions & 19 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,35 +221,47 @@ fn activate_deps<R: Registry>(cx: Context,
log!(5, "{}[{}]>{} trying {}", parent.get_name(), cur, dep.get_name(),
candidate.get_version());
let mut my_cx = cx.clone();
{
let early_return = {
my_cx.resolve.graph.link(parent.get_package_id().clone(),
candidate.get_package_id().clone());
let prev = match my_cx.activations.entry(key.clone()) {
Occupied(e) => e.into_mut(),
Vacant(e) => e.set(Vec::new()),
};
if !prev.iter().any(|c| c == candidate) {
if prev.iter().any(|c| c == candidate) {
match cx.resolve.features(candidate.get_package_id()) {
Some(prev_features) => {
features.iter().all(|f| prev_features.contains(f))
}
None => features.len() == 0,
}
} else {
my_cx.resolve.graph.add(candidate.get_package_id().clone(), []);
prev.push(candidate.clone());
false
}
}
};

// Dependency graphs are required to be a DAG. Non-transitive
// dependencies (dev-deps), however, can never introduce a cycle, so we
// skip them.
if dep.is_transitive() &&
!cx.visited.borrow_mut().insert(candidate.get_package_id().clone()) {
return Err(human(format!("cyclic package dependency: package `{}` \
depends on itself",
candidate.get_package_id())))
}
let my_cx = try!(activate(my_cx, registry, &**candidate, method));
if dep.is_transitive() {
cx.visited.borrow_mut().remove(candidate.get_package_id());
}
let my_cx = match my_cx {
Ok(cx) => cx,
Err(e) => { last_err = Some(e); continue }
let my_cx = if early_return {
my_cx
} else {
// Dependency graphs are required to be a DAG. Non-transitive
// dependencies (dev-deps), however, can never introduce a cycle, so we
// skip them.
if dep.is_transitive() &&
!cx.visited.borrow_mut().insert(candidate.get_package_id().clone()) {
return Err(human(format!("cyclic package dependency: package `{}` \
depends on itself",
candidate.get_package_id())))
}
let my_cx = try!(activate(my_cx, registry, &**candidate, method));
if dep.is_transitive() {
cx.visited.borrow_mut().remove(candidate.get_package_id());
}
match my_cx {
Ok(cx) => cx,
Err(e) => { last_err = Some(e); continue }
}
};
match try!(activate_deps(my_cx, registry, parent, deps, cur + 1)) {
Ok(cx) => return Ok(Ok(cx)),
Expand Down
8 changes: 1 addition & 7 deletions tests/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,7 @@ fn resolving_cycle() {
pkg!("foo" => ["foo"]),
));

let res = resolve(pkg_id("root"), vec![
let _ = resolve(pkg_id("root"), vec![
dep_req("foo", "1"),
], &mut reg);
assert!(res.is_err());

assert_eq!(res.to_string().as_slice(), "Err(\
cyclic package dependency: package `foo v1.0.0 (registry http://example.com/)` \
depends on itself\
)");
}
4 changes: 1 addition & 3 deletions tests/test_cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1091,9 +1091,7 @@ test!(self_dependency {
"#)
.file("src/test.rs", "fn main() {}");
assert_that(p.cargo_process("build"),
execs().with_status(101).with_stderr("\
cyclic package dependency: package `test v0.0.0 ([..])` depends on itself
"));
execs().with_status(0));
})

#[cfg(not(windows))]
Expand Down

4 comments on commit 7db89ed

@bors
Copy link
Collaborator

@bors bors commented on 7db89ed Oct 22, 2014

Choose a reason for hiding this comment

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

saw approval from wycats
at alexcrichton@7db89ed

@bors
Copy link
Collaborator

@bors bors commented on 7db89ed Oct 22, 2014

Choose a reason for hiding this comment

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

merging alexcrichton/cargo/souped-up-resolve = 7db89ed into auto-cargo

@bors
Copy link
Collaborator

@bors bors commented on 7db89ed Oct 22, 2014

Choose a reason for hiding this comment

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

alexcrichton/cargo/souped-up-resolve = 7db89ed merged ok, testing candidate = cdc85ff3

@bors
Copy link
Collaborator

@bors bors commented on 7db89ed Oct 22, 2014

Choose a reason for hiding this comment

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

Please sign in to comment.