From 78f356465321b55fed86868e380dc73d45401e1b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 24 Jan 2018 16:17:33 -0500 Subject: [PATCH 01/10] add the Manifest links attribute to Summary --- src/cargo/core/summary.rs | 14 ++++++++++---- src/cargo/sources/registry/index.rs | 4 ++-- src/cargo/sources/registry/mod.rs | 2 ++ src/cargo/util/toml/mod.rs | 2 +- tests/resolve.rs | 10 +++++----- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 734f73bd63b..81bb5c3d6f0 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -22,12 +22,14 @@ struct Inner { dependencies: Vec, features: BTreeMap>, checksum: Option, + links: Option, } impl Summary { pub fn new(pkg_id: PackageId, dependencies: Vec, - features: BTreeMap>) -> CargoResult { + features: BTreeMap>, + links: Option) -> CargoResult { for dep in dependencies.iter() { if features.get(dep.name()).is_some() { bail!("Features and dependencies cannot have the \ @@ -66,9 +68,10 @@ impl Summary { Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, - dependencies: dependencies, - features: features, + dependencies, + features, checksum: None, + links, }), }) } @@ -82,6 +85,9 @@ impl Summary { pub fn checksum(&self) -> Option<&str> { self.inner.checksum.as_ref().map(|s| &s[..]) } + pub fn links(&self) -> Option<&str> { + self.inner.links.as_ref().map(|s| &s[..]) + } pub fn override_id(mut self, id: PackageId) -> Summary { Rc::make_mut(&mut self.inner).package_id = id; @@ -94,7 +100,7 @@ impl Summary { } pub fn map_dependencies(mut self, f: F) -> Summary - where F: FnMut(Dependency) -> Dependency { + where F: FnMut(Dependency) -> Dependency { { let slot = &mut Rc::make_mut(&mut self.inner).dependencies; let deps = mem::replace(slot, Vec::new()); diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index f2e9932443d..053775d4d14 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -146,12 +146,12 @@ impl<'cfg> RegistryIndex<'cfg> { fn parse_registry_package(&mut self, line: &str) -> CargoResult<(Summary, bool)> { let RegistryPackage { - name, vers, cksum, deps, features, yanked + name, vers, cksum, deps, features, yanked, links } = super::DEFAULT_ID.set(&self.source_id, || { serde_json::from_str::(line) })?; let pkgid = PackageId::new(&name, &vers, &self.source_id)?; - let summary = Summary::new(pkgid, deps.inner, features)?; + let summary = Summary::new(pkgid, deps.inner, features, links)?; let summary = summary.set_checksum(cksum.clone()); if self.hashes.contains_key(&name[..]) { self.hashes.get_mut(&name[..]).unwrap().insert(vers, cksum); diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 3ef6e67fd3c..e7edf91f169 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -220,6 +220,8 @@ struct RegistryPackage<'a> { features: BTreeMap>, cksum: String, yanked: Option, + #[serde(default)] + links: Option, } struct DependencyList { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index f73790941d2..fddd6ccd308 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -672,7 +672,7 @@ impl TomlManifest { let include = project.include.clone().unwrap_or_default(); let summary = Summary::new(pkgid, deps, me.features.clone() - .unwrap_or_else(BTreeMap::new))?; + .unwrap_or_else(BTreeMap::new), project.links.clone())?; let metadata = ManifestMetadata { description: project.description.clone(), homepage: project.homepage.clone(), diff --git a/tests/resolve.rs b/tests/resolve.rs index 42a67dd37d0..aa89d59da7f 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -32,7 +32,7 @@ fn resolve(pkg: &PackageId, deps: Vec, registry: &[Summary]) fn requires_precise(&self) -> bool { false } } let mut registry = MyRegistry(registry); - let summary = Summary::new(pkg.clone(), deps, BTreeMap::new()).unwrap(); + let summary = Summary::new(pkg.clone(), deps, BTreeMap::new(), None).unwrap(); let method = Method::Everything; let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None, false)?; let res = resolve.iter().cloned().collect(); @@ -78,11 +78,11 @@ macro_rules! pkg { ($pkgid:expr => [$($deps:expr),+]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; - Summary::new($pkgid.to_pkgid(), d, BTreeMap::new()).unwrap() + Summary::new($pkgid.to_pkgid(), d, BTreeMap::new(), None).unwrap() }); ($pkgid:expr) => ( - Summary::new($pkgid.to_pkgid(), Vec::new(), BTreeMap::new()).unwrap() + Summary::new($pkgid.to_pkgid(), Vec::new(), BTreeMap::new(), None).unwrap() ) } @@ -92,7 +92,7 @@ fn registry_loc() -> SourceId { } fn pkg(name: &str) -> Summary { - Summary::new(pkg_id(name), Vec::new(), BTreeMap::new()).unwrap() + Summary::new(pkg_id(name), Vec::new(), BTreeMap::new(), None).unwrap() } fn pkg_id(name: &str) -> PackageId { @@ -108,7 +108,7 @@ fn pkg_id_loc(name: &str, loc: &str) -> PackageId { } fn pkg_loc(name: &str, loc: &str) -> Summary { - Summary::new(pkg_id_loc(name, loc), Vec::new(), BTreeMap::new()).unwrap() + Summary::new(pkg_id_loc(name, loc), Vec::new(), BTreeMap::new(), None).unwrap() } fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") } From 732d29dd6c566bb8f4c954ba6e067db4848c065b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 24 Jan 2018 21:28:31 -0500 Subject: [PATCH 02/10] Only allow one of each links attribute in resolver --- src/cargo/core/resolver/mod.rs | 50 ++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 02a26a9f79c..2e0cd5c0cf9 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -23,7 +23,9 @@ //! * Never try to activate a crate version which is incompatible. This means we //! only try crates which will actually satisfy a dependency and we won't ever //! try to activate a crate that's semver compatible with something else -//! activated (as we're only allowed to have one). +//! activated (as we're only allowed to have one) nor try to activate a crate +//! that has the same links attribute as something else +//! activated. //! * Always try to activate the highest version crate first. The default //! dependency in Cargo (e.g. when you write `foo = "0.1.2"`) is //! semver-compatible, so selecting the highest version possible will allow us @@ -376,7 +378,7 @@ pub fn resolve(summaries: &[(Summary, Method)], resolve_features: HashMap::new(), resolve_replacements: RcList::new(), activations: HashMap::new(), - replacements: replacements, + replacements, warnings: RcList::new(), }; let _p = profile::start("resolving"); @@ -476,7 +478,7 @@ impl RcVecIter { fn new(vec: Rc>) -> RcVecIter { RcVecIter { rest: 0..vec.len(), - vec: vec, + vec, } } } @@ -564,19 +566,21 @@ struct RemainingCandidates { } impl RemainingCandidates { - fn next(&mut self, prev_active: &[Summary]) -> Option { + fn next(&mut self, prev_active: &[Summary], links: &HashSet<&str>) -> Option { // 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. + // compatible unless we have a `*-sys` crate (defined by having a + // linked attribute) then we can only have one version. 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()) - }) + prev_active.iter().any(|a| *a == b.summary) + || (b.summary.links().map(|l| !links.contains(l)).unwrap_or(true) + && prev_active + .iter() + .all(|a| !compatible(a.version(), b.summary.version()))) }) } } @@ -664,6 +668,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, let (next, has_another, remaining_candidates) = { let prev_active = cx.prev_active(&dep); + let prev_links = cx.prev_links(); trace!("{}[{}]>{} {} candidates", parent.name(), cur, dep.name(), candidates.len()); trace!("{}[{}]>{} {} prev activations", parent.name(), cur, @@ -671,8 +676,8 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, let mut candidates = RemainingCandidates { remaining: RcVecIter::new(Rc::clone(&candidates)), }; - (candidates.next(prev_active), - candidates.clone().next(prev_active).is_some(), + (candidates.next(prev_active, &prev_links), + candidates.clone().next(prev_active, &prev_links).is_some(), candidates) }; @@ -682,7 +687,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, // 1. The version matches the dependency requirement listed for this // package // 2. There are no activated versions for this package which are - // semver-compatible, or there's an activated version which is + // semver/links-compatible, or there's an activated version which is // precisely equal to `candidate`. // // This means that we're going to attempt to activate each candidate in @@ -697,7 +702,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, cur, context_backup: Context::clone(&cx), deps_backup: >::clone(&remaining_deps), - remaining_candidates: remaining_candidates, + remaining_candidates, parent: Summary::clone(&parent), dep: Dependency::clone(&dep), features: Rc::clone(&features), @@ -758,8 +763,9 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, 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()) + let prev_links = frame.context_backup.prev_links(); + (frame.remaining_candidates.next(prev_active, &prev_links), + frame.remaining_candidates.clone().next(prev_active, &prev_links).is_some()) }; if let Some(candidate) = next { *cur = frame.cur; @@ -1033,9 +1039,9 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method) } impl<'a> Context<'a> { - // Activate this summary by inserting it into our list of known activations. - // - // Returns if this summary with the given method is already activated. + /// Activate this summary by inserting it into our list of known activations. + /// + /// Returns true if this summary with the given method is already activated. fn flag_activated(&mut self, summary: &Summary, method: &Method) -> bool { @@ -1178,6 +1184,14 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } + fn prev_links(&self) -> HashSet<&str> { + self.activations.iter() + .flat_map(|(_, v)| v.iter()) + .flat_map(|(_, v)| v.iter()) + .filter_map(|s| s.links()) + .collect() + } + /// Return all dependencies and the features we want from them. fn resolve_features<'b>(&mut self, s: &'b Summary, From 1ee77e48c2c567636a485829066292313c60178e Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 25 Jan 2018 22:50:55 -0500 Subject: [PATCH 03/10] Get ci passing --- tests/build-script.rs | 42 ++++++++++++------------------------------ 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/tests/build-script.rs b/tests/build-script.rs index 32ff94e9f98..936e2e2c9bc 100644 --- a/tests/build-script.rs +++ b/tests/build-script.rs @@ -227,6 +227,7 @@ not have a custom build script #[test] fn links_duplicates() { + // this tests that the links_duplicates are caught at resolver time let p = project("foo") .file("Cargo.toml", r#" [project] @@ -256,20 +257,15 @@ fn links_duplicates() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -[ERROR] multiple packages link to native library `a`, but a native library can \ -be linked only once - -package `foo v0.5.0 ([..])` -links to native library `a` - -package `a-sys v0.5.0 ([..])` - ... which is depended on by `foo v0.5.0 ([..])` -also links to native library `a` +error: failed to select a version for `a-sys` (required by `foo`): +all possible versions conflict with previously selected versions of `a-sys` + possible versions to select: 0.5.0 ")); } #[test] fn links_duplicates_deep_dependency() { + // this tests that the links_duplicates are caught at resolver time let p = project("foo") .file("Cargo.toml", r#" [project] @@ -311,16 +307,9 @@ fn links_duplicates_deep_dependency() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -[ERROR] multiple packages link to native library `a`, but a native library can \ -be linked only once - -package `foo v0.5.0 ([..])` -links to native library `a` - -package `a-sys v0.5.0 ([..])` - ... which is depended on by `a v0.5.0 ([..])` - ... which is depended on by `foo v0.5.0 ([..])` -also links to native library `a` +error: failed to select a version for `a-sys` (required by `a`): +all possible versions conflict with previously selected versions of `a-sys` + possible versions to select: 0.5.0 ")); } @@ -2741,6 +2730,7 @@ fn deterministic_rustc_dependency_flags() { #[test] fn links_duplicates_with_cycle() { + // this tests that the links_duplicates are caught at resolver time let p = project("foo") .file("Cargo.toml", r#" [project] @@ -2783,17 +2773,9 @@ fn links_duplicates_with_cycle() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -[ERROR] multiple packages link to native library `a`, but a native library can \ -be linked only once - -package `foo v0.5.0 ([..])` - ... which is depended on by `b v0.5.0 ([..])` -links to native library `a` - -package `a v0.5.0 (file://[..])` - ... which is depended on by `foo v0.5.0 ([..])` - ... which is depended on by `b v0.5.0 ([..])` -also links to native library `a` +error: failed to select a version for `a` (required by `foo`): +all possible versions conflict with previously selected versions of `a` + possible versions to select: 0.5.0 ")); } From 87d188f7a9bbb3f91c1a3a2a3302f1728c4b8dcb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 26 Jan 2018 15:19:49 -0500 Subject: [PATCH 04/10] Keep the list of links as part of Context --- src/cargo/core/resolver/mod.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 2e0cd5c0cf9..8fe8db58dba 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -347,11 +347,12 @@ enum GraphNode { // possible. #[derive(Clone)] struct Context<'a> { - // TODO: Both this and the map below are super expensive to clone. We should + // TODO: Both this and the map two below are super expensive to clone. We should // switch to persistent hash maps if we can at some point or otherwise // make these much cheaper to clone in general. activations: Activations, resolve_features: HashMap>, + links: HashSet, // 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 @@ -376,6 +377,7 @@ pub fn resolve(summaries: &[(Summary, Method)], let cx = Context { resolve_graph: RcList::new(), resolve_features: HashMap::new(), + links: HashSet::new(), resolve_replacements: RcList::new(), activations: HashMap::new(), replacements, @@ -566,7 +568,7 @@ struct RemainingCandidates { } impl RemainingCandidates { - fn next(&mut self, prev_active: &[Summary], links: &HashSet<&str>) -> Option { + fn next(&mut self, prev_active: &[Summary], links: &HashSet) -> Option { // 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 @@ -668,7 +670,6 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, let (next, has_another, remaining_candidates) = { let prev_active = cx.prev_active(&dep); - let prev_links = cx.prev_links(); trace!("{}[{}]>{} {} candidates", parent.name(), cur, dep.name(), candidates.len()); trace!("{}[{}]>{} {} prev activations", parent.name(), cur, @@ -676,8 +677,8 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, let mut candidates = RemainingCandidates { remaining: RcVecIter::new(Rc::clone(&candidates)), }; - (candidates.next(prev_active, &prev_links), - candidates.clone().next(prev_active, &prev_links).is_some(), + (candidates.next(prev_active, &cx.links), + candidates.clone().next(prev_active, &cx.links).is_some(), candidates) }; @@ -763,9 +764,8 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, while let Some(mut frame) = backtrack_stack.pop() { let (next, has_another) = { let prev_active = frame.context_backup.prev_active(&frame.dep); - let prev_links = frame.context_backup.prev_links(); - (frame.remaining_candidates.next(prev_active, &prev_links), - frame.remaining_candidates.clone().next(prev_active, &prev_links).is_some()) + (frame.remaining_candidates.next(prev_active, &frame.context_backup.links), + frame.remaining_candidates.clone().next(prev_active, &frame.context_backup.links).is_some()) }; if let Some(candidate) = next { *cur = frame.cur; @@ -1053,6 +1053,9 @@ impl<'a> Context<'a> { .or_insert(Vec::new()); if !prev.iter().any(|c| c == summary) { self.resolve_graph.push(GraphNode::Add(id.clone())); + if let Some(link) = summary.links() { + self.links.insert(link.to_owned()); + } prev.push(summary.clone()); return false } @@ -1184,14 +1187,6 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } - fn prev_links(&self) -> HashSet<&str> { - self.activations.iter() - .flat_map(|(_, v)| v.iter()) - .flat_map(|(_, v)| v.iter()) - .filter_map(|s| s.links()) - .collect() - } - /// Return all dependencies and the features we want from them. fn resolve_features<'b>(&mut self, s: &'b Summary, From 3fb2981af7c4ad183220f55172954d780faf1d99 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 27 Jan 2018 15:44:28 -0500 Subject: [PATCH 05/10] ensure we are not using links more then once --- src/cargo/core/resolver/mod.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8fe8db58dba..6084403ae02 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -440,13 +440,13 @@ fn activate(cx: &mut Context, candidate.summary.package_id().clone())); } - let activated = cx.flag_activated(&candidate.summary, method); + let activated = cx.flag_activated(&candidate.summary, method)?; let candidate = match candidate.replace { Some(replace) => { cx.resolve_replacements.push((candidate.summary.package_id().clone(), replace.package_id().clone())); - if cx.flag_activated(&replace, method) && activated { + if cx.flag_activated(&replace, method)? && activated { return Ok(None); } trace!("activating {} (replacing {})", replace.package_id(), @@ -1044,7 +1044,7 @@ impl<'a> Context<'a> { /// Returns true if this summary with the given method is already activated. fn flag_activated(&mut self, summary: &Summary, - method: &Method) -> bool { + method: &Method) -> CargoResult { let id = summary.package_id(); let prev = self.activations .entry(id.name().to_string()) @@ -1054,28 +1054,30 @@ impl<'a> Context<'a> { if !prev.iter().any(|c| c == summary) { self.resolve_graph.push(GraphNode::Add(id.clone())); if let Some(link) = summary.links() { - self.links.insert(link.to_owned()); + ensure!(self.links.insert(link.to_owned()), + "Attempting to resolve a with more then one crate with the links={}. \n\ + This will not build as is. Consider rebuilding the .lock file.", link); } prev.push(summary.clone()); - return false + return Ok(false) } debug!("checking if {} is already activated", summary.package_id()); let (features, use_default) = match *method { Method::Required { features, uses_default_features, .. } => { (features, uses_default_features) } - Method::Everything => return false, + Method::Everything => return Ok(false), }; let has_default_feature = summary.features().contains_key("default"); - match self.resolve_features.get(id) { + Ok(match self.resolve_features.get(id) { Some(prev) => { features.iter().all(|f| prev.contains(f)) && (!use_default || prev.contains("default") || !has_default_feature) } None => features.is_empty() && (!use_default || !has_default_feature) - } + }) } fn build_deps(&mut self, From 58590ae3c495bb82aa056c02929b01efcc63a3ba Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 27 Jan 2018 23:01:57 -0500 Subject: [PATCH 06/10] Send the links attribute to the registry --- src/cargo/core/manifest.rs | 1 + src/cargo/ops/registry.rs | 3 ++- src/cargo/util/toml/mod.rs | 1 + src/crates-io/lib.rs | 2 ++ 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index ad85eb0b7f4..fa6909bd3cf 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -77,6 +77,7 @@ pub struct ManifestMetadata { pub repository: Option, // url pub documentation: Option, // url pub badges: BTreeMap>, + pub links: Option, } #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 530519f6015..90c386839cf 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -159,7 +159,7 @@ fn transmit(config: &Config, let ManifestMetadata { ref authors, ref description, ref homepage, ref documentation, ref keywords, ref readme, ref repository, ref license, ref license_file, - ref categories, ref badges, + ref categories, ref badges, ref links, } = *manifest.metadata(); let readme_content = match *readme { Some(ref readme) => Some(paths::read(&pkg.root().join(readme))?), @@ -194,6 +194,7 @@ fn transmit(config: &Config, license: license.clone(), license_file: license_file.clone(), badges: badges.clone(), + links: links.clone(), }, tarball); match publish { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fddd6ccd308..4a1366e0fb2 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -685,6 +685,7 @@ impl TomlManifest { keywords: project.keywords.clone().unwrap_or_default(), categories: project.categories.clone().unwrap_or_default(), badges: me.badges.clone().unwrap_or_default(), + links: project.links.clone(), }; let workspace_config = match (me.workspace.as_ref(), diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index be284984779..19d3e700c60 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -55,6 +55,8 @@ pub struct NewCrate { pub license_file: Option, pub repository: Option, pub badges: BTreeMap>, + #[serde(default)] + pub links: Option, } #[derive(Serialize)] From affddaad8042b09d9e917c738a9323d19951662d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 30 Jan 2018 14:46:58 -0500 Subject: [PATCH 07/10] Add a test for issues/4902 --- tests/resolve.rs | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/tests/resolve.rs b/tests/resolve.rs index aa89d59da7f..8b6b4ca2b69 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -77,13 +77,17 @@ impl ToPkgId for (&'static str, &'static str) { macro_rules! pkg { ($pkgid:expr => [$($deps:expr),+]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; + let pkgid = $pkgid.to_pkgid(); + let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().to_string())} else {None}; - Summary::new($pkgid.to_pkgid(), d, BTreeMap::new(), None).unwrap() + Summary::new(pkgid, d, BTreeMap::new(), link).unwrap() }); - ($pkgid:expr) => ( - Summary::new($pkgid.to_pkgid(), Vec::new(), BTreeMap::new(), None).unwrap() - ) + ($pkgid:expr) => ({ + let pkgid = $pkgid.to_pkgid(); + let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().to_string())} else {None}; + Summary::new(pkgid, Vec::new(), BTreeMap::new(), link).unwrap() + }) } fn registry_loc() -> SourceId { @@ -92,7 +96,8 @@ fn registry_loc() -> SourceId { } fn pkg(name: &str) -> Summary { - Summary::new(pkg_id(name), Vec::new(), BTreeMap::new(), None).unwrap() + let link = if name.ends_with("-sys") {Some(name.to_string())} else {None}; + Summary::new(pkg_id(name), Vec::new(), BTreeMap::new(), link).unwrap() } fn pkg_id(name: &str) -> PackageId { @@ -108,7 +113,8 @@ fn pkg_id_loc(name: &str, loc: &str) -> PackageId { } fn pkg_loc(name: &str, loc: &str) -> Summary { - Summary::new(pkg_id_loc(name, loc), Vec::new(), BTreeMap::new(), None).unwrap() + let link = if name.ends_with("-sys") {Some(name.to_string())} else {None}; + Summary::new(pkg_id_loc(name, loc), Vec::new(), BTreeMap::new(), link).unwrap() } fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") } @@ -364,6 +370,33 @@ fn resolving_with_deep_backtracking() { ("baz", "1.0.1")]))); } +#[test] +fn resolving_with_sys_crates() { + // This is based on issues/4902 + // With `l` a normal library we get 2copies so everyone gets the newest compatible. + // But `l-sys` a library with a links attribute we make sure there is only one. + let reg = registry(vec![ + pkg!(("l-sys", "0.9.1")), + pkg!(("l-sys", "0.10.0")), + pkg!(("l", "0.9.1")), + pkg!(("l", "0.10.0")), + pkg!(("d", "1.0.0") => [dep_req("l-sys", ">=0.8.0, <=0.10.0"), dep_req("l", ">=0.8.0, <=0.10.0")]), + pkg!(("r", "1.0.0") => [dep_req("l-sys", "0.9"), dep_req("l", "0.9")]), + ]); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("d", "1"), + dep_req("r", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("root", "1.0.0"), + ("d", "1.0.0"), + ("r", "1.0.0"), + ("l-sys", "0.9.1"), + ("l", "0.9.1"), + ("l", "0.10.0")]))); +} + #[test] fn resolving_but_no_exists() { let reg = registry(vec![ From 50cd36ac4905eff55e36ffa72d3426960a41529d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 13 Feb 2018 17:56:04 -0500 Subject: [PATCH 08/10] Fix a==b and links, and update backtracking --- src/cargo/core/resolver/mod.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 7c28733a86b..423e99b83a7 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -332,7 +332,7 @@ struct Context<'a> { // make these much cheaper to clone in general. activations: Activations, resolve_features: HashMap>, - links: HashSet, + links: HashMap, // 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 @@ -357,7 +357,7 @@ pub fn resolve(summaries: &[(Summary, Method)], let cx = Context { resolve_graph: RcList::new(), resolve_features: HashMap::new(), - links: HashSet::new(), + links: HashMap::new(), resolve_replacements: RcList::new(), activations: HashMap::new(), replacements, @@ -551,7 +551,7 @@ struct RemainingCandidates { } impl RemainingCandidates { - fn next(&mut self, prev_active: &[Summary], links: &HashSet) -> Result> { + fn next(&mut self, prev_active: &[Summary], links: &HashMap) -> Result> { // 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 @@ -565,9 +565,11 @@ impl RemainingCandidates { // 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 b.summary.links().map(|l| links.contains(l)).unwrap_or(false) { - // TODO: self.conflicting_prev_active.insert(___.package_id().clone()); - continue + if let Some(a) = b.summary.links().and_then(|l| links.get(l)) { + if a != b.summary.package_id() { + self.conflicting_prev_active.insert(a.clone()); + continue + } } if let Some(a) = prev_active.iter().find(|a| compatible(a.version(), b.summary.version())) { if *a != b.summary { @@ -1077,7 +1079,7 @@ impl<'a> Context<'a> { if !prev.iter().any(|c| c == summary) { self.resolve_graph.push(GraphNode::Add(id.clone())); if let Some(link) = summary.links() { - ensure!(self.links.insert(link.to_owned()), + ensure!(self.links.insert(link.to_owned(), id.clone()).is_none(), "Attempting to resolve a with more then one crate with the links={}. \n\ This will not build as is. Consider rebuilding the .lock file.", link); } From ddccbca3c186a9e51ba87662ac136569e91bb776 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 13 Feb 2018 23:11:48 -0500 Subject: [PATCH 09/10] Add an explanation of links related conflicts to error messages --- src/cargo/core/resolver/mod.rs | 46 +++++++++++++++++++++++----------- tests/build-script.rs | 22 +++++++++++----- 2 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 423e99b83a7..9f7068dbc33 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -532,6 +532,12 @@ impl Ord for DepsFrame { } } +#[derive(Clone, PartialOrd, Ord, PartialEq, Eq)] +enum ConflictReason { + Semver, + Links(String), +} + struct BacktrackFrame<'a> { cur: usize, context_backup: Context<'a>, @@ -540,18 +546,18 @@ struct BacktrackFrame<'a> { parent: Summary, dep: Dependency, features: Rc>, - conflicting_activations: HashSet, + conflicting_activations: HashMap, } #[derive(Clone)] struct RemainingCandidates { remaining: RcVecIter, // note: change to RcList or something if clone is to expensive - conflicting_prev_active: HashSet, + conflicting_prev_active: HashMap, } impl RemainingCandidates { - fn next(&mut self, prev_active: &[Summary], links: &HashMap) -> Result> { + fn next(&mut self, prev_active: &[Summary], links: &HashMap) -> Result> { // 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 @@ -565,15 +571,17 @@ impl RemainingCandidates { // 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) = b.summary.links().and_then(|l| links.get(l)) { - if a != b.summary.package_id() { - self.conflicting_prev_active.insert(a.clone()); - continue + if let Some(link) = b.summary.links() { + if let Some(a) = links.get(link) { + if a != b.summary.package_id() { + self.conflicting_prev_active.insert(a.clone(), ConflictReason::Links(link.to_owned())); + continue + } } } 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()); + self.conflicting_prev_active.insert(a.package_id().clone(), ConflictReason::Semver); continue } } @@ -673,9 +681,9 @@ 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_prev_active: HashMap::new(), }; - conflicting_activations = HashSet::new(); + conflicting_activations = HashMap::new(); (candidates.next(prev_active, &cx.links), candidates.clone().next(prev_active, &cx.links).is_ok(), candidates) @@ -772,7 +780,7 @@ fn find_candidate<'a>( cur: &mut usize, dep: &mut Dependency, features: &mut Rc>, - conflicting_activations: &mut HashSet, + conflicting_activations: &mut HashMap, ) -> Option { while let Some(mut frame) = backtrack_stack.pop() { let (next, has_another) = { @@ -786,7 +794,7 @@ fn find_candidate<'a>( && conflicting_activations .iter() // note: a lot of redundant work in is_active for similar debs - .all(|con| frame.context_backup.is_active(con)) + .all(|(con, _)| frame.context_backup.is_active(con)) { continue; } @@ -818,7 +826,7 @@ fn activation_error(cx: &Context, registry: &mut Registry, parent: &Summary, dep: &Dependency, - conflicting_activations: HashSet, + conflicting_activations: HashMap, candidates: &[Candidate], config: Option<&Config>) -> CargoError { let graph = cx.graph(); @@ -842,9 +850,17 @@ fn activation_error(cx: &Context, msg.push_str(&describe_path(parent.package_id())); let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect(); conflicting_activations.sort_unstable(); - for v in conflicting_activations.iter().rev() { + for &(p, r) in conflicting_activations.iter().rev() { + match r { + &ConflictReason::Links(ref link) => { + msg.push_str("\n multiple packages link to native library `"); + msg.push_str(link); + msg.push_str("`, but a native library can be linked only once.") + }, + _ => (), + } msg.push_str("\n previously selected "); - msg.push_str(&describe_path(v)); + msg.push_str(&describe_path(p)); } msg.push_str("\n possible versions to select: "); diff --git a/tests/build-script.rs b/tests/build-script.rs index 936e2e2c9bc..41ffb7eeb7f 100644 --- a/tests/build-script.rs +++ b/tests/build-script.rs @@ -257,8 +257,11 @@ fn links_duplicates() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -error: failed to select a version for `a-sys` (required by `foo`): -all possible versions conflict with previously selected versions of `a-sys` +error: failed to select a version for `a-sys`. +all possible versions conflict with previously selected packages. +required by package `foo v0.5.0 ([..])` + multiple packages link to native library `a`, but a native library can be linked only once. + previously selected package `foo v0.5.0 ([..])` possible versions to select: 0.5.0 ")); } @@ -307,8 +310,12 @@ fn links_duplicates_deep_dependency() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -error: failed to select a version for `a-sys` (required by `a`): -all possible versions conflict with previously selected versions of `a-sys` +error: failed to select a version for `a-sys`. +all possible versions conflict with previously selected packages. +required by package `a v0.5.0 ([..])` + ... which is depended on by `foo v0.5.0 ([..])` + multiple packages link to native library `a`, but a native library can be linked only once. + previously selected package `foo v0.5.0 ([..])` possible versions to select: 0.5.0 ")); } @@ -2773,8 +2780,11 @@ fn links_duplicates_with_cycle() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr("\ -error: failed to select a version for `a` (required by `foo`): -all possible versions conflict with previously selected versions of `a` +error: failed to select a version for `a`. +all possible versions conflict with previously selected packages. +required by package `foo v0.5.0 ([..])` + multiple packages link to native library `a`, but a native library can be linked only once. + previously selected package `foo v0.5.0 ([..])` possible versions to select: 0.5.0 ")); } From 70dada8af02ac5f25143fd244e55224ae39e629c Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 14 Feb 2018 22:28:59 -0500 Subject: [PATCH 10/10] better error messages --- src/cargo/core/resolver/mod.rs | 60 +++++++++++++++++++++++++--------- tests/build-script.rs | 36 +++++++++++--------- tests/build.rs | 19 ++++++++--- 3 files changed, 79 insertions(+), 36 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 45a8747b262..2345a59da42 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -538,6 +538,15 @@ enum ConflictReason { Links(String), } +impl ConflictReason { + fn is_links(&self) -> bool { + match self { + &ConflictReason::Semver => false, + &ConflictReason::Links(_) => true, + } + } +} + struct BacktrackFrame<'a> { cur: usize, context_backup: Context<'a>, @@ -835,33 +844,52 @@ fn activation_error(cx: &Context, dep_path_desc }; if !candidates.is_empty() { - let mut msg = format!("failed to select a version for `{}`.\n\ - all possible versions conflict with \ - previously selected packages.\n", - dep.name()); - msg.push_str("required by "); + let mut msg = format!("failed to select a version for `{}`.", dep.name()); + msg.push_str("\n ... required by "); msg.push_str(&describe_path(parent.package_id())); + + msg.push_str("\nversions that meet the requirements `"); + msg.push_str(&dep.version_req().to_string()); + msg.push_str("` are: "); + msg.push_str(&candidates.iter() + .map(|v| v.summary.version()) + .map(|v| v.to_string()) + .collect::>() + .join(", ")); + let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect(); conflicting_activations.sort_unstable(); - for &(p, r) in conflicting_activations.iter().rev() { + let (links_errors, other_errors): (Vec<_>, Vec<_>) = conflicting_activations.drain(..).rev().partition(|&(_, r)| r.is_links()); + + for &(p, r) in &links_errors { match r { &ConflictReason::Links(ref link) => { - msg.push_str("\n multiple packages link to native library `"); - msg.push_str(link); - msg.push_str("`, but a native library can be linked only once.") + msg.push_str("\n\nthe package `"); + msg.push_str(dep.name()); + msg.push_str("` links to the native library `"); + msg.push_str(&link); + msg.push_str("`, but it conflicts with a previous package which links to `"); + msg.push_str(&link); + msg.push_str("` as well:\n"); }, _ => (), } - msg.push_str("\n previously selected "); msg.push_str(&describe_path(p)); } - msg.push_str("\n possible versions to select: "); - msg.push_str(&candidates.iter() - .map(|v| v.summary.version()) - .map(|v| v.to_string()) - .collect::>() - .join(", ")); + if links_errors.is_empty() { + msg.push_str("\n\nall possible versions conflict with \ + previously selected packages."); + } + + for &(p, _) in &other_errors { + msg.push_str("\n\n previously selected "); + msg.push_str(&describe_path(p)); + } + + msg.push_str("\n\nfailed to select a version for `"); + msg.push_str(dep.name()); + msg.push_str("` which could resolve this conflict"); return format_err!("{}", msg) } diff --git a/tests/build-script.rs b/tests/build-script.rs index 41ffb7eeb7f..1bd0409cff5 100644 --- a/tests/build-script.rs +++ b/tests/build-script.rs @@ -258,11 +258,13 @@ fn links_duplicates() { execs().with_status(101) .with_stderr("\ error: failed to select a version for `a-sys`. -all possible versions conflict with previously selected packages. -required by package `foo v0.5.0 ([..])` - multiple packages link to native library `a`, but a native library can be linked only once. - previously selected package `foo v0.5.0 ([..])` - possible versions to select: 0.5.0 + ... required by package `foo v0.5.0 ([..])` +versions that meet the requirements `*` are: 0.5.0 + +the package `a-sys` links to the native library `a`, but it conflicts with a previous package which links to `a` as well: +package `foo v0.5.0 ([..])` + +failed to select a version for `a-sys` which could resolve this conflict ")); } @@ -311,12 +313,14 @@ fn links_duplicates_deep_dependency() { execs().with_status(101) .with_stderr("\ error: failed to select a version for `a-sys`. -all possible versions conflict with previously selected packages. -required by package `a v0.5.0 ([..])` + ... required by package `a v0.5.0 ([..])` ... which is depended on by `foo v0.5.0 ([..])` - multiple packages link to native library `a`, but a native library can be linked only once. - previously selected package `foo v0.5.0 ([..])` - possible versions to select: 0.5.0 +versions that meet the requirements `*` are: 0.5.0 + +the package `a-sys` links to the native library `a`, but it conflicts with a previous package which links to `a` as well: +package `foo v0.5.0 ([..])` + +failed to select a version for `a-sys` which could resolve this conflict ")); } @@ -2781,11 +2785,13 @@ fn links_duplicates_with_cycle() { execs().with_status(101) .with_stderr("\ error: failed to select a version for `a`. -all possible versions conflict with previously selected packages. -required by package `foo v0.5.0 ([..])` - multiple packages link to native library `a`, but a native library can be linked only once. - previously selected package `foo v0.5.0 ([..])` - possible versions to select: 0.5.0 + ... required by package `foo v0.5.0 ([..])` +versions that meet the requirements `*` are: 0.5.0 + +the package `a` links to the native library `a`, but it conflicts with a previous package which links to `a` as well: +package `foo v0.5.0 ([..])` + +failed to select a version for `a` which could resolve this conflict ")); } diff --git a/tests/build.rs b/tests/build.rs index 7f3e887be49..47a7cd4b1e9 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -1027,13 +1027,17 @@ fn incompatible_dependencies() { 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 `baz v0.1.0` + ... required by package `baz v0.1.0` ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` +versions that meet the requirements `>= 1.0.1` are: 1.0.2, 1.0.1 + +all possible versions conflict with previously selected packages. + 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")); + +failed to select a version for `bad` which could resolve this conflict")); } #[test] @@ -1063,15 +1067,20 @@ fn incompatible_dependencies_with_multi_semver() { execs().with_status(101) .with_stderr_contains("\ error: failed to select a version for `bad`. + ... required by package `incompatible_dependencies v0.0.1 ([..])` +versions that meet the requirements `>= 1.0.1, <= 2.0.0` are: 2.0.0, 1.0.1 + 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")); + +failed to select a version for `bad` which could resolve this conflict")); } #[test]