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.

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

part of the infrastructure for public & private dependencies in the resolver #6653

Merged
merged 13 commits into from
Mar 6, 2019
12 changes: 12 additions & 0 deletions src/cargo/core/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ struct Inner {
explicit_name_in_toml: Option<InternedString>,

optional: bool,
public: bool,
default_features: bool,
features: Vec<InternedString>,

Expand Down Expand Up @@ -217,6 +218,7 @@ impl Dependency {
kind: Kind::Normal,
only_match_name: true,
optional: false,
public: false,
features: Vec::new(),
default_features: true,
specified_req: false,
Expand Down Expand Up @@ -293,6 +295,16 @@ impl Dependency {
self.inner.kind
}

pub fn is_public(&self) -> bool {
self.inner.public
}

/// Sets whether the dependency is public.
pub fn set_public(&mut self, public: bool) -> &mut Dependency {
Rc::make_mut(&mut self.inner).public = public;
self
}

pub fn specified_req(&self) -> bool {
self.inner.specified_req
}
Expand Down
29 changes: 13 additions & 16 deletions src/cargo/core/resolver/conflict_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ use std::collections::{BTreeMap, HashMap, HashSet};

use log::trace;

use super::types::ConflictReason;
use super::types::{ConflictMap, ConflictReason};
use crate::core::resolver::Context;
use crate::core::{Dependency, PackageId};

/// This is a trie for storing a large number of sets designed to
/// efficiently see if any of the stored sets are a subset of a search set.
enum ConflictStoreTrie {
/// One of the stored sets.
Leaf(BTreeMap<PackageId, ConflictReason>),
Leaf(ConflictMap),
/// A map from an element to a subtrie where
/// all the sets in the subtrie contains that element.
Node(BTreeMap<PackageId, ConflictStoreTrie>),
Expand All @@ -23,7 +23,7 @@ impl ConflictStoreTrie {
&self,
cx: &Context,
must_contain: Option<PackageId>,
) -> Option<&BTreeMap<PackageId, ConflictReason>> {
) -> Option<&ConflictMap> {
match self {
ConflictStoreTrie::Leaf(c) => {
if must_contain.is_none() {
Expand Down Expand Up @@ -57,18 +57,14 @@ impl ConflictStoreTrie {
}
}

fn insert(
&mut self,
mut iter: impl Iterator<Item = PackageId>,
con: BTreeMap<PackageId, ConflictReason>,
) {
fn insert(&mut self, mut iter: impl Iterator<Item = PackageId>, con: ConflictMap) {
if let Some(pid) = iter.next() {
if let ConflictStoreTrie::Node(p) = self {
p.entry(pid)
.or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
.insert(iter, con);
}
// Else, we already have a subset of this in the `ConflictStore`.
// Else, we already have a subset of this in the `ConflictStore`.
} else {
// We are at the end of the set we are adding, there are three cases for what to do
// next:
Expand Down Expand Up @@ -147,7 +143,7 @@ impl ConflictCache {
cx: &Context,
dep: &Dependency,
must_contain: Option<PackageId>,
) -> Option<&BTreeMap<PackageId, ConflictReason>> {
) -> Option<&ConflictMap> {
let out = self
.con_from_dep
.get(dep)?
Expand All @@ -161,18 +157,19 @@ impl ConflictCache {
}
out
}
pub fn conflicting(
&self,
cx: &Context,
dep: &Dependency,
) -> Option<&BTreeMap<PackageId, ConflictReason>> {
pub fn conflicting(&self, cx: &Context, dep: &Dependency) -> Option<&ConflictMap> {
self.find_conflicting(cx, dep, None)
}

/// Adds to the cache a conflict of the form:
/// `dep` is known to be unresolvable if
/// all the `PackageId` entries are activated.
pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
pub fn insert(&mut self, dep: &Dependency, con: &ConflictMap) {
if con.values().any(|c| *c == ConflictReason::PublicDependency) {
// TODO: needs more info for back jumping
// for now refuse to cache it.
return;
}
self.con_from_dep
.entry(dep.clone())
.or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
Expand Down
29 changes: 25 additions & 4 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::{BTreeMap, HashMap, HashSet};
use std::collections::{HashMap, HashSet};
use std::rc::Rc;

// "ensure" seems to require "bail" be in scope (macro hygiene issue?).
Expand All @@ -12,7 +12,9 @@ use crate::util::CargoResult;
use crate::util::Graph;

use super::errors::ActivateResult;
use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
use super::types::{
ConflictMap, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer,
};

pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
pub use super::encode::{Metadata, WorkspaceResolve};
Expand All @@ -25,8 +27,20 @@ pub use super::resolve::Resolve;
#[derive(Clone)]
pub struct Context {
pub activations: Activations,
/// list the features that are activated for each package
pub resolve_features: im_rc::HashMap<PackageId, Rc<HashSet<InternedString>>>,
/// get the package that will be linking to a native library by its links attribute
pub links: im_rc::HashMap<InternedString, PackageId>,
/// for each package the list of names it can see,
/// then for each name the exact version that name represents and weather the name is public.
pub public_dependency:
Option<im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>>,

// This is somewhat redundant with the `resolve_graph` that stores the same data,
// but for querying in the opposite order.
/// a way to look up for a package in activations what packages required it
/// and all of the exact deps that it fulfilled.
pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
Copy link
Member

Choose a reason for hiding this comment

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

In our past debugging we've found that cloning a Context happens so often that an expensive clone means a slower resolver, so I was curious about this!

I think we explicitly removed Graph from this data structure earlier in favor of RcList below, so was curious on your thoughts on this.

I haven't read the whole patch yet though, so I'll likely find what happens soon.


// These are two cheaply-cloneable lists (O(1) clone) which are effectively
// hash maps but are built up as "construction lists". We'll iterate these
Expand All @@ -38,14 +52,21 @@ pub struct Context {
pub warnings: RcList<String>,
}

/// list all the activated versions of a particular crate name from a source
pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;

impl Context {
pub fn new() -> Context {
pub fn new(check_public_visible_dependencies: bool) -> Context {
Context {
resolve_graph: RcList::new(),
resolve_features: im_rc::HashMap::new(),
links: im_rc::HashMap::new(),
public_dependency: if check_public_visible_dependencies {
Some(im_rc::HashMap::new())
} else {
None
},
parents: Graph::new(),
resolve_replacements: RcList::new(),
activations: im_rc::HashMap::new(),
warnings: RcList::new(),
Expand Down Expand Up @@ -147,7 +168,7 @@ impl Context {
pub fn is_conflicting(
&self,
parent: Option<PackageId>,
conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
conflicting_activations: &ConflictMap,
) -> bool {
conflicting_activations
.keys()
Expand Down
26 changes: 15 additions & 11 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::collections::BTreeMap;
use std::fmt;

use crate::core::{Dependency, PackageId, Registry, Summary};
Expand All @@ -8,7 +7,7 @@ use failure::{Error, Fail};
use semver;

use super::context::Context;
use super::types::{Candidate, ConflictReason};
use super::types::{Candidate, ConflictMap, ConflictReason};

/// Error during resolution providing a path of `PackageId`s.
pub struct ResolveError {
Expand Down Expand Up @@ -73,16 +72,15 @@ pub(super) fn activation_error(
registry: &mut dyn Registry,
parent: &Summary,
dep: &Dependency,
conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
conflicting_activations: &ConflictMap,
candidates: &[Candidate],
config: Option<&Config>,
) -> ResolveError {
let graph = cx.graph();
let to_resolve_err = |err| {
ResolveError::new(
err,
graph
.path_to_top(&parent.package_id())
cx.parents
.path_to_bottom(&parent.package_id())
.into_iter()
.cloned()
.collect(),
Expand All @@ -92,7 +90,9 @@ pub(super) fn activation_error(
if !candidates.is_empty() {
let mut msg = format!("failed to select a version for `{}`.", dep.package_name());
msg.push_str("\n ... required by ");
msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(
&cx.parents.path_to_bottom(&parent.package_id()),
));

msg.push_str("\nversions that meet the requirements `");
msg.push_str(&dep.version_req().to_string());
Expand Down Expand Up @@ -123,7 +123,7 @@ pub(super) fn activation_error(
msg.push_str(link);
msg.push_str("` as well:\n");
}
msg.push_str(&describe_path(&graph.path_to_top(p)));
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
}

let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
Expand Down Expand Up @@ -154,7 +154,7 @@ pub(super) fn activation_error(

for &(p, _) in other_errors.iter() {
msg.push_str("\n\n previously selected ");
msg.push_str(&describe_path(&graph.path_to_top(p)));
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
}

msg.push_str("\n\nfailed to select a version for `");
Expand Down Expand Up @@ -204,7 +204,9 @@ pub(super) fn activation_error(
registry.describe_source(dep.source_id()),
);
msg.push_str("required by ");
msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(
&cx.parents.path_to_bottom(&parent.package_id()),
));

// If we have a path dependency with a locked version, then this may
// indicate that we updated a sub-package and forgot to run `cargo
Expand Down Expand Up @@ -265,7 +267,9 @@ pub(super) fn activation_error(
msg.push_str("\n");
}
msg.push_str("required by ");
msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(
&cx.parents.path_to_bottom(&parent.package_id()),
));

msg
};
Expand Down
Loading