diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 7071d88d1aa..7ab01ab4fd0 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -9,11 +9,11 @@ use log::debug; use crate::core::interning::InternedString; use crate::core::{Dependency, PackageId, SourceId, Summary}; -use crate::util::CargoResult; use crate::util::Graph; use super::dep_cache::RegistryQueryer; -use super::types::{ConflictMap, FeaturesSet, Method}; +use super::types::{ConflictMap, ConflictReason, FeaturesSet, Method}; +use super::ActivateResult; pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve}; pub use super::encode::{Metadata, WorkspaceResolve}; @@ -100,25 +100,34 @@ impl Context { /// Activate this summary by inserting it into our list of known activations. /// /// Returns `true` if this summary with the given method is already activated. - pub fn flag_activated(&mut self, summary: &Summary, method: &Method) -> CargoResult { + pub fn flag_activated(&mut self, summary: &Summary, method: &Method) -> ActivateResult { let id = summary.package_id(); let age: ContextAge = self.age(); match self.activations.entry(id.as_activations_key()) { im_rc::hashmap::Entry::Occupied(o) => { - debug_assert_eq!( - &o.get().0, - summary, - "cargo does not allow two semver compatible versions" - ); + let other = o.get().0.package_id(); + // Cargo dictates that you can't duplicate multiple + // semver-compatible versions of a crate. For example we can't + // simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can, + // however, activate `1.0.2` and `2.0.0`. + // + // Here we throw out our candidate if it's *compatible*, yet not + // equal, to all previously activated versions. + if other != summary.package_id() { + return Err((other, ConflictReason::Semver).into()); + } } im_rc::hashmap::Entry::Vacant(v) => { if let Some(link) = summary.links() { - ensure!( - self.links.insert(link, id).is_none(), - "Attempting to resolve a dependency with more then one crate with the \ - links={}.\nThis will not build as is. Consider rebuilding the .lock file.", - &*link - ); + if let Some(other) = self.links.insert(link, id) { + // The `links` key in the manifest dictates that there's only one + // package in a dependency graph, globally, with that particular + // `links` key. If this candidate links to something that's already + // linked to by a different package then we've gotta skip this. + if other != summary.package_id() { + return Err((other, ConflictReason::Links(link)).into()); + } + } } v.insert((summary.clone(), age)); return Ok(false); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index c410b7accca..926ec6c1074 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -48,7 +48,6 @@ //! over the place. use std::collections::{BTreeMap, HashMap, HashSet}; -use std::mem; use std::rc::Rc; use std::time::{Duration, Instant}; @@ -208,16 +207,15 @@ fn activate_deps_loop( while let Some((just_here_for_the_error_messages, frame)) = remaining_deps.pop_most_constrained() { - let (mut parent, (mut cur, (mut dep, candidates, mut features))) = frame; + let (mut parent, (mut dep, candidates, mut features)) = frame; // If we spend a lot of time here (we shouldn't in most cases) then give // a bit of a visual indicator as to what we're doing. printed.shell_status(config)?; trace!( - "{}[{}]>{} {} candidates", + "{}>{} {} candidates", parent.name(), - cur, dep.package_name(), candidates.len() ); @@ -227,7 +225,7 @@ fn activate_deps_loop( .conflicting(&cx, &dep) .is_some(); - let mut remaining_candidates = RemainingCandidates::new(&candidates); + let mut remaining_candidates = RcVecIter::new(candidates.clone()); // `conflicting_activations` stores all the reasons we were unable to // activate candidates. One of these reasons will have to go away for @@ -246,26 +244,16 @@ fn activate_deps_loop( let mut backtracked = false; loop { - let next = remaining_candidates.next( - &mut conflicting_activations, - &cx, - &dep, - parent.package_id(), - ); + let next = remaining_candidates.next(); - let (candidate, has_another) = next.ok_or(()).or_else(|_| { + let candidate = next.ok_or(()).or_else(|_| { // If we get here then our `remaining_candidates` was just // exhausted, so `dep` failed to activate. // // It's our job here to backtrack, if possible, and find a // different candidate to activate. If we can't find any // candidates whatsoever then it's time to bail entirely. - trace!( - "{}[{}]>{} -- no candidates", - parent.name(), - cur, - dep.package_name() - ); + trace!("{}>{} -- no candidates", parent.name(), dep.package_name()); // Use our list of `conflicting_activations` to add to our // global list of past conflicting activations, effectively @@ -305,10 +293,9 @@ fn activate_deps_loop( .as_ref() .unwrap_or(&conflicting_activations), ) { - Some((candidate, has_another, frame)) => { + Some((candidate, frame)) => { // Reset all of our local variables used with the // contents of `frame` to complete our backtrack. - cur = frame.cur; cx = frame.context; remaining_deps = frame.remaining_deps; remaining_candidates = frame.remaining_candidates; @@ -317,7 +304,7 @@ fn activate_deps_loop( features = frame.features; conflicting_activations = frame.conflicting_activations; backtracked = true; - Ok((candidate, has_another)) + Ok(candidate) } None => { debug!("no candidates found"); @@ -339,7 +326,10 @@ fn activate_deps_loop( // more candidates we want to fast-forward to the last one as // otherwise we'll just backtrack here anyway (helping us to skip // some work). - if just_here_for_the_error_messages && !backtracked && has_another { + if just_here_for_the_error_messages + && !backtracked + && remaining_candidates.has_another() + { continue; } @@ -351,9 +341,8 @@ fn activate_deps_loop( // frame. This is a relatively important optimization as a number of // the `clone` calls below can be quite expensive, so we avoid them // if we can. - let backtrack = if has_another { + let backtrack = if remaining_candidates.has_another() { Some(BacktrackFrame { - cur, context: Context::clone(&cx), remaining_deps: remaining_deps.clone(), remaining_candidates: remaining_candidates.clone(), @@ -374,9 +363,8 @@ fn activate_deps_loop( uses_default_features: dep.uses_default_features(), }; trace!( - "{}[{}]>{} trying {}", + "{}>{} trying {}", parent.name(), - cur, dep.package_name(), candidate.version() ); @@ -409,7 +397,7 @@ fn activate_deps_loop( if let Some(conflicting) = frame .remaining_siblings .clone() - .filter_map(|(_, (ref new_dep, _, _))| { + .filter_map(|(ref new_dep, _, _)| { past_conflicting_activations.conflicting(&cx, new_dep) }) .next() @@ -488,18 +476,19 @@ fn activate_deps_loop( // prune extra work). If we don't have any candidates in our // backtrack stack then we're the last line of defense, so // we'll want to present an error message for sure. - let activate_for_error_message = has_past_conflicting_dep && !has_another && { - just_here_for_the_error_messages || { - find_candidate( - &cx, - &mut backtrack_stack.clone(), - &parent, - backtracked, - &conflicting_activations, - ) - .is_none() - } - }; + let activate_for_error_message = + has_past_conflicting_dep && !remaining_candidates.has_another() && { + just_here_for_the_error_messages || { + find_candidate( + &cx, + &mut backtrack_stack.clone(), + &parent, + backtracked, + &conflicting_activations, + ) + .is_none() + } + }; // If we're only here for the error messages then we know // one of our candidate deps will fail, meaning we will @@ -524,9 +513,8 @@ fn activate_deps_loop( true } else { trace!( - "{}[{}]>{} skipping {} ", + "{}>{} skipping {} ", parent.name(), - cur, dep.package_name(), pid.version() ); @@ -622,8 +610,11 @@ fn activate( while let Some((p, public)) = stack.pop() { match public_dependency.entry(p).or_default().entry(c.name()) { im_rc::hashmap::Entry::Occupied(mut o) => { - // the (transitive) parent can already see something by `c`s name, it had better be `c`. - assert_eq!(o.get().0, c); + if o.get().0 != c { + // the (transitive) parent can already see a different version by `t`s name. + // So, adding `b` will cause `p` to have a public dependency conflict on `c`. + return Err((p, ConflictReason::PublicDependency).into()); + } if o.get().1 { // The previous time the parent saw `c`, it was a public dependency. // So all of its parents already know about `c` @@ -700,153 +691,15 @@ fn activate( #[derive(Clone)] struct BacktrackFrame { - cur: usize, context: Context, remaining_deps: RemainingDeps, - remaining_candidates: RemainingCandidates, + remaining_candidates: RcVecIter, parent: Summary, dep: Dependency, features: FeaturesSet, conflicting_activations: ConflictMap, } -/// A helper "iterator" used to extract candidates within a current `Context` of -/// a dependency graph. -/// -/// This struct doesn't literally implement the `Iterator` trait (requires a few -/// more inputs) but in general acts like one. Each `RemainingCandidates` is -/// created with a list of candidates to choose from. When attempting to iterate -/// over the list of candidates only *valid* candidates are returned. Validity -/// is defined within a `Context`. -/// -/// Candidates passed to `new` may not be returned from `next` as they could be -/// filtered out, and as they are filtered the causes will be added to `conflicting_prev_active`. -#[derive(Clone)] -struct RemainingCandidates { - remaining: RcVecIter, - // This is a inlined peekable generator - has_another: Option, -} - -impl RemainingCandidates { - fn new(candidates: &Rc>) -> RemainingCandidates { - RemainingCandidates { - remaining: RcVecIter::new(Rc::clone(candidates)), - has_another: None, - } - } - - /// Attempts to find another candidate to check from this list. - /// - /// This method will attempt to move this iterator forward, returning a - /// candidate that's possible to activate. The `cx` argument is the current - /// context which determines validity for candidates returned, and the `dep` - /// is the dependency listing that we're activating for. - /// - /// If successful a `(Candidate, bool)` pair will be returned. The - /// `Candidate` is the candidate to attempt to activate, and the `bool` is - /// an indicator of whether there are remaining candidates to try of if - /// we've reached the end of iteration. - /// - /// If we've reached the end of the iterator here then `Err` will be - /// returned. The error will contain a map of package ID to conflict reason, - /// where each package ID caused a candidate to be filtered out from the - /// original list for the reason listed. - fn next( - &mut self, - conflicting_prev_active: &mut ConflictMap, - cx: &Context, - dep: &Dependency, - parent: PackageId, - ) -> Option<(Summary, bool)> { - 'main: for (_, b) in self.remaining.by_ref() { - let b_id = b.package_id(); - // The `links` key in the manifest dictates that there's only one - // package in a dependency graph, globally, with that particular - // `links` key. If this candidate links to something that's already - // linked to by a different package then we've gotta skip this. - if let Some(link) = b.links() { - if let Some(&a) = cx.links.get(&link) { - if a != b_id { - conflicting_prev_active - .entry(a) - .or_insert_with(|| ConflictReason::Links(link)); - continue; - } - } - } - - // Otherwise the condition for being a valid candidate relies on - // semver. Cargo dictates that you can't duplicate multiple - // semver-compatible versions of a crate. For example we can't - // simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can, - // however, activate `1.0.2` and `2.0.0`. - // - // Here we throw out our candidate if it's *compatible*, yet not - // equal, to all previously activated versions. - if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) { - if *a != b { - conflicting_prev_active - .entry(a.package_id()) - .or_insert(ConflictReason::Semver); - continue; - } - } - // We may still have to reject do to a public dependency conflict. If one of any of our - // ancestors that can see us already knows about a different crate with this name then - // we have to reject this candidate. Additionally this candidate may already have been - // activated and have public dependants of its own, - // all of witch also need to be checked the same way. - if let Some(public_dependency) = cx.public_dependency.as_ref() { - let existing_public_deps: Vec = public_dependency - .get(&b_id) - .iter() - .flat_map(|x| x.values()) - .filter_map(|x| if x.1 { Some(&x.0) } else { None }) - .chain(&Some(b_id)) - .cloned() - .collect(); - for t in existing_public_deps { - // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent, dep.is_public())]; - while let Some((p, public)) = stack.pop() { - // TODO: dont look at the same thing more then once - if let Some(o) = public_dependency.get(&p).and_then(|x| x.get(&t.name())) { - if o.0 != t { - // the (transitive) parent can already see a different version by `t`s name. - // So, adding `b` will cause `p` to have a public dependency conflict on `t`. - conflicting_prev_active.insert(p, ConflictReason::PublicDependency); - continue 'main; - } - } - // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` - if public { - // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in cx.parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); - } - } - } - } - } - - // Well if we made it this far then we've got a valid dependency. We - // want this iterator to be inherently "peekable" so we don't - // necessarily return the item just yet. Instead we stash it away to - // get returned later, and if we replaced something then that was - // actually the candidate to try first so we return that. - if let Some(r) = mem::replace(&mut self.has_another, Some(b)) { - return Some((r, true)); - } - } - - // Alright we've entirely exhausted our list of candidates. If we've got - // something stashed away return that here (also indicating that there's - // nothing else). - self.has_another.take().map(|r| (r, false)) - } -} - /// Attempts to find a new conflict that allows a `find_candidate` feather then the input one. /// It will add the new conflict to the cache if one is found. /// @@ -969,7 +822,7 @@ fn find_candidate( parent: &Summary, backtracked: bool, conflicting_activations: &ConflictMap, -) -> Option<(Summary, bool, BacktrackFrame)> { +) -> Option<(Summary, BacktrackFrame)> { // When we're calling this method we know that `parent` failed to // activate. That means that some dependency failed to get resolved for // whatever reason. Normally, that means that all of those reasons @@ -992,14 +845,9 @@ fn find_candidate( }; while let Some(mut frame) = backtrack_stack.pop() { - let next = frame.remaining_candidates.next( - &mut frame.conflicting_activations, - &frame.context, - &frame.dep, - frame.parent.package_id(), - ); - let (candidate, has_another) = match next { - Some(pair) => pair, + let next = frame.remaining_candidates.next(); + let candidate = match next { + Some(s) => s, None => continue, }; @@ -1035,7 +883,7 @@ fn find_candidate( } } - return Some((candidate, has_another, frame)); + return Some((candidate, frame)); } None } diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index ea8d3a22c43..58a03bd6eaf 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -139,14 +139,14 @@ impl DepsFrame { fn min_candidates(&self) -> usize { self.remaining_siblings .peek() - .map(|(_, (_, candidates, _))| candidates.len()) + .map(|(_, candidates, _)| candidates.len()) .unwrap_or(0) } pub fn flatten<'a>(&'a self) -> impl Iterator + 'a { self.remaining_siblings .clone() - .map(move |(_, (d, _, _))| (self.parent.package_id(), d)) + .map(move |(d, _, _)| (self.parent.package_id(), d)) } } @@ -202,7 +202,7 @@ impl RemainingDeps { self.data.insert((x, insertion_time)); self.time += 1; } - pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> { + pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, DepInfo))> { while let Some((mut deps_frame, insertion_time)) = self.data.remove_min() { let just_here_for_the_error_messages = deps_frame.just_for_error_messages; @@ -303,11 +303,12 @@ impl RcVecIter { } } - fn peek(&self) -> Option<(usize, &T)> { - self.rest - .clone() - .next() - .and_then(|i| self.vec.get(i).map(|val| (i, &*val))) + pub fn has_another(&self) -> bool { + self.rest.start < self.rest.end + } + + fn peek(&self) -> Option<&T> { + self.rest.clone().next().and_then(|i| self.vec.get(i)) } } @@ -325,12 +326,10 @@ impl Iterator for RcVecIter where T: Clone, { - type Item = (usize, T); + type Item = T; fn next(&mut self) -> Option { - self.rest - .next() - .and_then(|i| self.vec.get(i).map(|val| (i, val.clone()))) + self.rest.next().and_then(|i| self.vec.get(i).cloned()) } fn size_hint(&self) -> (usize, Option) {