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.

Sign up for GitHub

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

Jump to bottom

Conflict tracking #5037

Merged
merged 3 commits into from Feb 14, 2018
Merged

Conflict tracking #5037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
201fdba
More directly track conflicts in backtracking, hopefully this will be…
Eh2406 Feb 11, 2018
a028221
Use the conflict tracking in the error messages to only show the vers…
Eh2406 Feb 13, 2018
8899093
Don't store conflicting_activations for backtracking.
Eh2406 Feb 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
139 changes: 82 additions & 57 deletions src/cargo/core/resolver/mod.rs
View file
Open in desktop
Expand Up @@ -536,28 +536,39 @@ struct BacktrackFrame<'a> {
parent: Summary,
dep: Dependency,
features: Rc<Vec<String>>,
conflicting_activations: HashSet<PackageId>,
}

#[derive(Clone)]
struct RemainingCandidates {
remaining: RcVecIter<Candidate>,
// note: change to RcList or something if clone is to expensive
conflicting_prev_active: HashSet<PackageId>,
}

impl RemainingCandidates {
fn next(&mut self, prev_active: &[Summary]) -> Option<Candidate> {
fn next(&mut self, prev_active: &[Summary]) -> Result<Candidate, HashSet<PackageId>> {
// Filter the set of candidates based on the previously activated
// versions for this dependency. We can actually use a version if it
// precisely matches an activated version or if it is otherwise
// incompatible with all other activated versions. Note that we
// define "compatible" here in terms of the semver sense where if
// the left-most nonzero digit is the same they're considered
// compatible.
self.remaining.by_ref().map(|p| p.1).find(|b| {
prev_active.iter().any(|a| *a == b.summary) ||
prev_active.iter().all(|a| {
!compatible(a.version(), b.summary.version())
})
})
//
// When we are done we return the set of previously activated
// that conflicted with the ones we tried. If any of these change
// then we would have considered different candidates.
for (_, b) in self.remaining.by_ref() {
if let Some(a) = prev_active.iter().find(|a| compatible(a.version(), b.summary.version())) {
if *a != b.summary {
self.conflicting_prev_active.insert(a.package_id().clone());
continue
}
}
return Ok(b);
}
Err(self.conflicting_prev_active.clone())
}
}

Expand All @@ -580,6 +591,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
// use (those with more candidates).
let mut backtrack_stack = Vec::new();
let mut remaining_deps = BinaryHeap::new();
let mut conflicting_activations;
for &(ref summary, ref method) in summaries {
debug!("initial activation: {}", summary.package_id());
let candidate = Candidate { summary: summary.clone(), replace: None };
Expand Down Expand Up @@ -650,9 +662,11 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
dep.name(), prev_active.len());
let mut candidates = RemainingCandidates {
remaining: RcVecIter::new(Rc::clone(&candidates)),
conflicting_prev_active: HashSet::new(),
};
conflicting_activations = HashSet::new();
(candidates.next(prev_active),
candidates.clone().next(prev_active).is_some(),
candidates.clone().next(prev_active).is_ok(),
candidates)
};

Expand All @@ -669,7 +683,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
// turn. We could possibly fail to activate each candidate, so we try
// each one in turn.
let candidate = match next {
Some(candidate) => {
Ok(candidate) => {
// We have a candidate. Add an entry to the `backtrack_stack` so
// we can try the next one if this one fails.
if has_another {
Expand All @@ -681,11 +695,13 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
parent: Summary::clone(&parent),
dep: Dependency::clone(&dep),
features: Rc::clone(&features),
conflicting_activations: conflicting_activations.clone(),
Copy link
Member

@aidanhs aidanhs Feb 14, 2018 •
edited

Choose a reason for hiding this comment

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

I'm 90% sure this is always empty - the only place we add to conflicting_activations is when we've run out of candidates, in which case we're about to backtrack...which means conflicting_activations immediately gets overwritten with an empty set again.

This makes sense, because when backtracking you only care about the conflicts for the current dep rather than for the whole resolve history. It also simplifies the code significantly because we don't need to keep it in backtrack frames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, conflicting_activations is at this time mostly unused. It is used to move conflicting to find_candidate and then use to shuttle the info to activation_error. The intent is to allow anything to add to the conflicting_activations at any point in the proses. (specifically for #5000). It would be reasonable to remove it in this PR and add it back in in that PR iif it turns out to be useful there.

Copy link
Member

Choose a reason for hiding this comment

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

@Eh2406 that sounds like a good strategy to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

});
}
candidate
}
None => {
Err(mut conflicting) => {
conflicting_activations.extend(conflicting.drain());
// This dependency has no valid candidate. Backtrack until we
// find a dependency that does have a candidate to try, and try
// to activate that one. This resets the `remaining_deps` to
Expand All @@ -698,10 +714,11 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
&mut parent,
&mut cur,
&mut dep,
&mut features) {
&mut features,
&mut conflicting_activations) {
None => return Err(activation_error(&cx, registry, &parent,
&dep,
cx.prev_active(&dep),
conflicting_activations,
&candidates, config)),
Some(candidate) => candidate,
}
Expand All @@ -725,55 +742,62 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
Ok(cx)
}

// Looks through the states in `backtrack_stack` for dependencies with
// remaining candidates. For each one, also checks if rolling back
// could change the outcome of the failed resolution that caused backtracking
// in the first place - namely, if we've backtracked past the parent of the
// failed dep, or the previous number of activations of the failed dep has
// changed (possibly relaxing version constraints). If the outcome could differ,
// resets `cx` and `remaining_deps` to that level and returns the
// next candidate. If all candidates have been exhausted, returns None.
// Read https://github.com/rust-lang/cargo/pull/4834#issuecomment-362871537 for
// a more detailed explanation of the logic here.
fn find_candidate<'a>(backtrack_stack: &mut Vec<BacktrackFrame<'a>>,
cx: &mut Context<'a>,
remaining_deps: &mut BinaryHeap<DepsFrame>,
parent: &mut Summary,
cur: &mut usize,
dep: &mut Dependency,
features: &mut Rc<Vec<String>>) -> Option<Candidate> {
let num_dep_prev_active = cx.prev_active(dep).len();
/// Looks through the states in `backtrack_stack` for dependencies with
/// remaining candidates. For each one, also checks if rolling back
/// could change the outcome of the failed resolution that caused backtracking
/// in the first place. Namely, if we've backtracked past the parent of the
/// failed dep, or any of the packages flagged as giving us trouble in conflicting_activations.
/// Read https://github.com/rust-lang/cargo/pull/4834
/// For several more detailed explanations of the logic here.
///
/// If the outcome could differ, resets `cx` and `remaining_deps` to that
/// level and returns the next candidate.
/// If all candidates have been exhausted, returns None.
fn find_candidate<'a>(
backtrack_stack: &mut Vec<BacktrackFrame<'a>>,
cx: &mut Context<'a>,
remaining_deps: &mut BinaryHeap<DepsFrame>,
parent: &mut Summary,
cur: &mut usize,
dep: &mut Dependency,
features: &mut Rc<Vec<String>>,
conflicting_activations: &mut HashSet<PackageId>,
) -> Option<Candidate> {
while let Some(mut frame) = backtrack_stack.pop() {
let (next, has_another) = {
let prev_active = frame.context_backup.prev_active(&frame.dep);
(frame.remaining_candidates.next(prev_active),
frame.remaining_candidates.clone().next(prev_active).is_some())
(
frame.remaining_candidates.next(prev_active),
frame.remaining_candidates.clone().next(prev_active).is_ok(),
)
};
let cur_num_dep_prev_active = frame.context_backup.prev_active(dep).len();
// Activations should monotonically decrease during backtracking
assert!(cur_num_dep_prev_active <= num_dep_prev_active);
let maychange = !frame.context_backup.is_active(parent) ||
cur_num_dep_prev_active != num_dep_prev_active;
if !maychange {
continue
if frame.context_backup.is_active(parent.package_id())
&& conflicting_activations
.iter()
// note: a lot of redundant work in is_active for similar debs
.all(|con| frame.context_backup.is_active(con))
{
continue;
}
if let Some(candidate) = next {
if let Ok(candidate) = next {
*cur = frame.cur;
if has_another {
*cx = frame.context_backup.clone();
*remaining_deps = frame.deps_backup.clone();
*parent = frame.parent.clone();
*dep = frame.dep.clone();
*features = Rc::clone(&frame.features);
*conflicting_activations = frame.conflicting_activations.clone();
backtrack_stack.push(frame);
} else {
*cx = frame.context_backup;
*remaining_deps = frame.deps_backup;
*parent = frame.parent;
*dep = frame.dep;
*features = frame.features;
*conflicting_activations = frame.conflicting_activations
}
return Some(candidate)
return Some(candidate);
}
}
None
Expand All @@ -783,7 +807,7 @@ fn activation_error(cx: &Context,
registry: &mut Registry,
parent: &Summary,
dep: &Dependency,
prev_active: &[Summary],
conflicting_activations: HashSet<PackageId>,
candidates: &[Candidate],
config: Option<&Config>) -> CargoError {
let graph = cx.graph();
Expand All @@ -799,29 +823,31 @@ fn activation_error(cx: &Context,
dep_path_desc
};
if !candidates.is_empty() {
let mut msg = format!("failed to select a version for `{0}`\n\
let mut msg = format!("failed to select a version for `{}`.\n\
all possible versions conflict with \
previously selected versions of `{0}`\n",
previously selected packages.\n",
dep.name());
msg.push_str("required by ");
msg.push_str(&describe_path(parent.package_id()));
for v in prev_active.iter() {
let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect();
conflicting_activations.sort_unstable();
for v in conflicting_activations.iter().rev() {
msg.push_str("\n previously selected ");
msg.push_str(&describe_path(v.package_id()));
msg.push_str(&describe_path(v));
}

msg.push_str(&format!("\n possible versions to select: {}",
candidates.iter()
.map(|v| v.summary.version())
.map(|v| v.to_string())
.collect::<Vec<_>>()
.join(", ")));
msg.push_str("\n possible versions to select: ");
msg.push_str(&candidates.iter()
.map(|v| v.summary.version())
.map(|v| v.to_string())
.collect::<Vec<_>>()
.join(", "));

return format_err!("{}", msg)
}

// Once we're all the way down here, we're definitely lost in the
// weeds! We didn't actually use any candidates above, so we need to
// weeds! We didn't actually find any candidates, so we need to
// give an error message that nothing was found.
//
// Note that we re-query the registry with a new dependency that
Expand All @@ -834,7 +860,7 @@ fn activation_error(cx: &Context,
Ok(candidates) => candidates,
Err(e) => return e,
};
candidates.sort_by(|a, b| {
candidates.sort_unstable_by(|a, b| {
b.version().cmp(a.version())
});

Expand Down Expand Up @@ -1172,11 +1198,10 @@ impl<'a> Context<'a> {
.unwrap_or(&[])
}

fn is_active(&mut self, summary: &Summary) -> bool {
let id = summary.package_id();
fn is_active(&mut self, id: &PackageId) -> bool {
self.activations.get(id.name())
.and_then(|v| v.get(id.source_id()))
.map(|v| v.iter().any(|s| s == summary))
.map(|v| v.iter().any(|s| s.package_id() == id))
.unwrap_or(false)
}

Expand Down
45 changes: 40 additions & 5 deletions tests/build.rs
View file
Open in desktop
Expand Up @@ -1026,19 +1026,54 @@ fn incompatible_dependencies() {
assert_that(p.cargo("build"),
execs().with_status(101)
.with_stderr_contains("\
error: failed to select a version for `bad`
all possible versions conflict with previously selected versions of `bad`
error: failed to select a version for `bad`.
all possible versions conflict with previously selected packages.
required by package `baz v0.1.0`
... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
previously selected package `bad v0.1.0`
... which is depended on by `foo v0.1.0`
... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
previously selected package `bad v1.0.0`
... which is depended on by `bar v0.1.0`
... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
possible versions to select: 1.0.2, 1.0.1"));
}

#[test]
fn incompatible_dependencies_with_multi_semver() {
Package::new("bad", "1.0.0").publish();
Package::new("bad", "1.0.1").publish();
Package::new("bad", "2.0.0").publish();
Package::new("bad", "2.0.1").publish();
Package::new("bar", "0.1.0").dep("bad", "=1.0.0").publish();
Package::new("baz", "0.1.0").dep("bad", ">=2.0.1").publish();

let p = project("transitive_load_test")
.file("Cargo.toml", r#"
[project]
name = "incompatible_dependencies"
version = "0.0.1"

[dependencies]
bar = "0.1.0"
baz = "0.1.0"
bad = ">=1.0.1, <=2.0.0"
"#)
.file("src/main.rs", "fn main(){}")
.build();

assert_that(p.cargo("build"),
execs().with_status(101)
.with_stderr_contains("\
error: failed to select a version for `bad`.
all possible versions conflict with previously selected packages.
required by package `incompatible_dependencies v0.0.1 ([..])`
previously selected package `bad v2.0.1`
... which is depended on by `baz v0.1.0`
... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
previously selected package `bad v1.0.0`
... which is depended on by `bar v0.1.0`
... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
possible versions to select: 2.0.0, 1.0.1"));
}

#[test]
fn compile_offline_while_transitive_dep_not_cached() {
let bar = Package::new("bar", "1.0.0");
Expand Down