Skip to content

Commit

Permalink
Auto merge of #6653 - Eh2406:min-pub-dep, r=alexcrichton
Browse files Browse the repository at this point in the history
part of the infrastructure for public & private dependencies in the resolver

This is part of my work on public & private dependencies in the resolver from #6129. As discussed there the proptest fuzzers are happy to find exponential blow up with all the back jumping strategies I have tried. So this PR does not have a back jumping strategie nor does it have the proptest fuzzers generating public dependencies. These will both need to change for the feature to stabilize. In the meantime it gives the correct results on the cases it can handle.

With rust-lang/rust#57586 landed there is a lot of work to do on Cargos front end. Adding a UI for this, passing the relevant things to rustc, passing it to crates.io, passing it to cargo-metadata. This is good enough to allow that work to proceed.
  • Loading branch information
bors committed Mar 6, 2019
2 parents 0d2aac7 + 12e474b commit cb0f763
Show file tree
Hide file tree
Showing 11 changed files with 411 additions and 86 deletions.
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>>>,

// 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

0 comments on commit cb0f763

Please sign in to comment.