From e143491356e14d7767e38a638209476c4a57ae92 Mon Sep 17 00:00:00 2001 From: Yehuda Katz + Carl Lerche Date: Wed, 9 Jul 2014 17:48:37 -0700 Subject: [PATCH] Resolve more correctly and add --extern This commit adds support for passing --extern to dependencies. It allows multiple copies of the same named dependency in the system, as long as they come from different repos. --- src/cargo/core/manifest.rs | 7 ++ src/cargo/core/mod.rs | 1 + src/cargo/core/package_id.rs | 14 +++- src/cargo/core/resolver.rs | 144 +++++++++++++++++++++++++-------- src/cargo/lib.rs | 2 + src/cargo/ops/cargo_compile.rs | 14 ++-- src/cargo/ops/cargo_rustc.rs | 56 ++++++++++--- src/cargo/util/graph.rs | 24 +++++- src/cargo/util/mod.rs | 1 + tests/test_cargo_compile.rs | 4 +- 10 files changed, 209 insertions(+), 58 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 3eee27591..4871815d7 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -298,6 +298,13 @@ impl Manifest { } impl Target { + pub fn file_stem(&self) -> String { + match self.metadata { + Some(ref metadata) => format!("{}{}", self.name, metadata.extra_filename), + None => self.name.clone() + } + } + pub fn lib_target(name: &str, crate_targets: Vec, src_path: &Path, profile: &Profile, metadata: &Metadata) diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index 23716f6be..80fb64eed 100644 --- a/src/cargo/core/mod.rs +++ b/src/cargo/core/mod.rs @@ -42,6 +42,7 @@ pub use self::dependency::{ }; pub use self::version_req::VersionReq; +pub use self::resolver::Resolve; pub mod errors; pub mod source; diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 03236aa36..8e31bfe92 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -1,7 +1,9 @@ use semver; use url::Url; +use std::hash::Hash; use std::fmt; use std::fmt::{Show,Formatter}; +use collections::hash; use serialize::{ Encodable, Encoder, @@ -60,6 +62,16 @@ pub struct PackageId { source_id: SourceId, } +impl Hash for PackageId { + fn hash(&self, state: &mut S) { + self.name.hash(state); + self.version.to_str().hash(state); + self.source_id.hash(state); + } +} + +impl Eq for PackageId {} + #[deriving(Clone, Show, PartialEq)] pub enum PackageIdError { InvalidVersion(String), @@ -110,7 +122,7 @@ impl PackageId { let extra_filename = short_hash( &(self.name.as_slice(), self.version.to_str(), &self.source_id)); - Metadata { metadata: metadata, extra_filename: extra_filename } + Metadata { metadata: metadata, extra_filename: format!("-{}", extra_filename) } } } diff --git a/src/cargo/core/resolver.rs b/src/cargo/core/resolver.rs index 6a50c7108..607cee781 100644 --- a/src/cargo/core/resolver.rs +++ b/src/cargo/core/resolver.rs @@ -1,59 +1,128 @@ use std::collections::HashMap; +use util::graph::{Nodes,Edges}; use core::{ Dependency, PackageId, Summary, Registry, + SourceId, }; -use util::{CargoResult, human, internal}; +use semver; -/* TODO: - * - The correct input here is not a registry. Resolves should be performable - * on package summaries vs. the packages themselves. - */ -pub fn resolve(deps: &[Dependency], - registry: &mut R) -> CargoResult> { +use util::{CargoResult, Graph, human, internal}; + +pub struct Resolve { + graph: Graph +} + +impl Resolve { + fn new() -> Resolve { + Resolve { graph: Graph::new() } + } + + pub fn iter<'a>(&'a self) -> Nodes<'a, PackageId> { + self.graph.iter() + } + + pub fn deps<'a>(&'a self, pkg: &PackageId) -> Option> { + self.graph.edges(pkg) + } +} + +/* +pub fn resolve(deps: &[Dependency], registry: &mut R) -> CargoResult> { + Ok(try!(resolve2(deps, registry)).iter().map(|p| p.clone()).collect()) +} +*/ + +struct Context<'a, R> { + registry: &'a mut R, + resolve: Resolve, + + // Eventually, we will have smarter logic for checking for conflicts in the resolve, + // but without the registry, conflicts should not exist in practice, so this is just + // a sanity check. + seen: HashMap<(String, SourceId), semver::Version> +} + +impl<'a, R: Registry> Context<'a, R> { + fn new(registry: &'a mut R) -> Context<'a, R> { + Context { + registry: registry, + resolve: Resolve::new(), + seen: HashMap::new() + } + } +} + +pub fn resolve(deps: &[Dependency], registry: &mut R) -> CargoResult { log!(5, "resolve; deps={}", deps); - let mut remaining = Vec::from_slice(deps); - let mut resolve = HashMap::::new(); + let mut context = Context::new(registry); + try!(resolve_deps(None, deps, &mut context)); + Ok(context.resolve) +} - loop { - let curr = match remaining.pop() { - Some(curr) => curr, - None => { - let ret = resolve.values().map(|summary| { - summary.get_package_id().clone() - }).collect(); - log!(5, "resolve complete; ret={}", ret); - return Ok(ret); +fn resolve_deps<'a, R: Registry>(parent: Option<&PackageId>, + deps: &[Dependency], + ctx: &mut Context<'a, R>) + -> CargoResult<()> +{ + if deps.is_empty() { + return Ok(()); + } + + for dep in deps.iter() { + let pkgs = try!(ctx.registry.query(dep)); + + if pkgs.is_empty() { + return Err(human(format!("No package named {} found", dep))); + } + + if pkgs.len() > 1 { + return Err(internal(format!("At the moment, Cargo only supports a \ + single source for a particular package name ({}).", dep))); + } + + let summary = pkgs.get(0).clone(); + let name = summary.get_name().to_str(); + let source_id = summary.get_source_id().clone(); + let version = summary.get_version().clone(); + + let found = { + let found = ctx.seen.find(&(name.clone(), source_id.clone())); + + if found.is_some() { + if found == Some(&version) { continue; } + return Err(human(format!("Cargo found multiple copies of {} in {}. This \ + is not currently supported", + summary.get_name(), summary.get_source_id()))); + } else { + false } }; - let opts = try!(registry.query(&curr)); - - if opts.len() == 0 { - return Err(human(format!("No package named {} found", curr.get_name()))); + if !found { + ctx.seen.insert((name, source_id), version); } - if opts.len() > 1 { - return Err(internal(format!("At the moment, Cargo only supports a \ - single source for a particular package name ({}).", curr.get_name()))); - } + ctx.resolve.graph.add(summary.get_package_id().clone(), []); - let pkg = opts.get(0).clone(); - resolve.insert(pkg.get_name().to_str(), pkg.clone()); + let deps: Vec = summary.get_dependencies().iter() + .filter(|d| d.is_transitive()) + .map(|d| d.clone()) + .collect(); - for dep in pkg.get_dependencies().iter() { - if !dep.is_transitive() { continue; } + try!(resolve_deps(Some(summary.get_package_id()), deps.as_slice(), ctx)); - if !resolve.contains_key_equiv(&dep.get_name()) { - remaining.push(dep.clone()); - } - } + parent.map(|parent| { + ctx.resolve.graph.link(parent.clone(), summary.get_package_id().clone()); + }); } + + Ok(()) } #[cfg(test)] @@ -61,8 +130,13 @@ mod test { use hamcrest::{assert_that, equal_to, contains}; use core::source::{SourceId, RegistryKind, Location, Remote}; - use core::{Dependency, PackageId, Summary}; + use core::{Dependency, PackageId, Summary, Registry}; use super::resolve; + use util::CargoResult; + + fn resolve(deps: &[Dependency], registry: &mut R) -> CargoResult> { + Ok(try!(super::resolve(deps, registry)).iter().map(|p| p.clone()).collect()) + } trait ToDep { fn to_dep(self) -> Dependency; diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 32a07fd9c..259839ca3 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -2,9 +2,11 @@ #![crate_type="rlib"] #![feature(macro_rules, phase)] +#![feature(default_type_params)] extern crate debug; extern crate term; +extern crate collections; extern crate url; extern crate serialize; extern crate semver; diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index e8f13b3a1..aae4c0d62 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -24,7 +24,7 @@ use std::os; use util::config::{Config, ConfigValue}; -use core::{MultiShell, Source, SourceId, PackageSet, Target, resolver}; +use core::{MultiShell, Source, SourceId, PackageSet, Target, PackageId, resolver}; use core::registry::PackageRegistry; use ops; use sources::{PathSource}; @@ -57,7 +57,7 @@ pub fn compile(manifest_path: &Path, options: CompileOptions) -> CargoResult<()> let override_ids = try!(source_ids_from_config()); let source_ids = package.get_source_ids(); - let packages = { + let (packages, resolve) = { let mut config = try!(Config::new(shell, update, jobs)); let mut registry = @@ -66,9 +66,12 @@ pub fn compile(manifest_path: &Path, options: CompileOptions) -> CargoResult<()> let resolved = try!(resolver::resolve(package.get_dependencies(), &mut registry)); - try!(registry.get(resolved.as_slice()).wrap({ + let req: Vec = resolved.iter().map(|r| r.clone()).collect(); + let packages = try!(registry.get(req.as_slice()).wrap({ human("Unable to get packages from source") - })) + })); + + (packages, resolved) }; debug!("packages={}", packages); @@ -78,8 +81,9 @@ pub fn compile(manifest_path: &Path, options: CompileOptions) -> CargoResult<()> }).collect::>(); let mut config = try!(Config::new(shell, update, jobs)); + try!(ops::compile_targets(env.as_slice(), targets.as_slice(), &package, - &PackageSet::new(packages.as_slice()), &mut config)); + &PackageSet::new(packages.as_slice()), &resolve, &mut config)); Ok(()) } diff --git a/src/cargo/ops/cargo_rustc.rs b/src/cargo/ops/cargo_rustc.rs index 4b52dd951..7764a8858 100644 --- a/src/cargo/ops/cargo_rustc.rs +++ b/src/cargo/ops/cargo_rustc.rs @@ -6,7 +6,7 @@ use std::os::args; use std::str; use term::color::YELLOW; -use core::{Package, PackageSet, Target}; +use core::{Package, PackageSet, Target, Resolve}; use util; use util::{CargoResult, ChainError, ProcessBuilder, internal, human, CargoError}; use util::{Config, TaskPool, DependencyQueue, Fresh, Dirty, Freshness}; @@ -18,6 +18,8 @@ struct Context<'a, 'b> { deps_dir: &'a Path, primary: bool, rustc_version: &'a str, + resolve: &'a Resolve, + package_set: &'a PackageSet, config: &'b mut Config<'b> } @@ -42,9 +44,9 @@ fn uniq_target_dest<'a>(targets: &[&'a Target]) -> Option<&'a str> { } pub fn compile_targets<'a>(env: &str, targets: &[&Target], pkg: &Package, - deps: &PackageSet, - config: &'a mut Config<'a>) -> CargoResult<()> { - + deps: &PackageSet, resolve: &'a Resolve, + config: &'a mut Config<'a>) -> CargoResult<()> +{ if targets.is_empty() { return Ok(()); } @@ -74,6 +76,8 @@ pub fn compile_targets<'a>(env: &str, targets: &[&Target], pkg: &Package, deps_dir: &deps_target_dir, primary: false, rustc_version: rustc_version.as_slice(), + resolve: resolve, + package_set: deps, config: config }; @@ -135,7 +139,7 @@ fn compile(targets: &[&Target], pkg: &Package, // After the custom command has run, execute rustc for all targets of our // package. for &target in targets.iter() { - cmds.push(rustc(&pkg.get_root(), target, cx)); + cmds.push(rustc(pkg, target, cx)); } cmds.push(proc() { @@ -207,15 +211,16 @@ fn compile_custom(pkg: &Package, cmd: &str, proc() p.exec_with_output().map(|_| ()).map_err(|e| e.mark_human()) } -fn rustc(root: &Path, target: &Target, cx: &mut Context) -> Job { +fn rustc(package: &Package, target: &Target, cx: &mut Context) -> Job { let crate_types = target.rustc_crate_types(); + let root = package.get_root(); log!(5, "root={}; target={}; crate_types={}; dest={}; deps={}; verbose={}", root.display(), target, crate_types, cx.dest.display(), cx.deps_dir.display(), cx.primary); let primary = cx.primary; - let rustc = prepare_rustc(root, target, crate_types, cx); + let rustc = prepare_rustc(package, target, crate_types, cx); log!(5, "command={}", rustc); @@ -232,12 +237,14 @@ fn rustc(root: &Path, target: &Target, cx: &mut Context) -> Job { } } -fn prepare_rustc(root: &Path, target: &Target, crate_types: Vec<&str>, - cx: &Context) -> ProcessBuilder { +fn prepare_rustc(package: &Package, target: &Target, crate_types: Vec<&str>, + cx: &Context) -> ProcessBuilder +{ + let root = package.get_root(); let mut args = Vec::new(); build_base_args(&mut args, target, crate_types, cx); - build_deps_args(&mut args, cx); + build_deps_args(&mut args, package, cx); util::process("rustc") .cwd(root.clone()) @@ -286,7 +293,7 @@ fn build_base_args(into: &mut Args, into.push(format!("metadata={}", m.metadata)); into.push("-C".to_str()); - into.push(format!("extra-filename=-{}", m.extra_filename)); + into.push(format!("extra-filename={}", m.extra_filename)); } None => {} } @@ -300,11 +307,36 @@ fn build_base_args(into: &mut Args, } } -fn build_deps_args(dst: &mut Args, cx: &Context) { +fn build_deps_args(dst: &mut Args, package: &Package, cx: &Context) { dst.push("-L".to_str()); dst.push(cx.dest.display().to_str()); dst.push("-L".to_str()); dst.push(cx.deps_dir.display().to_str()); + + for target in dep_targets(package, cx).iter() { + dst.push("--extern".to_str()); + dst.push(format!("{}={}/lib{}.rlib", + target.get_name(), + cx.deps_dir.display(), + target.file_stem())); + } +} + +fn dep_targets(pkg: &Package, cx: &Context) -> Vec { + match cx.resolve.deps(pkg.get_package_id()) { + None => vec!(), + Some(deps) => deps + .map(|pkg_id| { + cx.package_set.iter() + .find(|pkg| pkg_id == pkg.get_package_id()) + .expect("Should have found package") + }) + .filter_map(|pkg| { + pkg.get_targets().iter().find(|&t| t.is_lib() && t.get_profile().is_compile()) + }) + .map(|t| t.clone()) + .collect() + } } /// Execute all jobs necessary to build the dependency graph. diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 7fc62aa3c..e8c57f339 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,8 +1,9 @@ use std::hash::Hash; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; +use std::collections::hashmap::{Keys, SetItems}; pub struct Graph { - nodes: HashMap> + nodes: HashMap> } enum Mark { @@ -10,13 +11,26 @@ enum Mark { Done } +pub type Nodes<'a, N> = Keys<'a, N, HashSet>; +pub type Edges<'a, N> = SetItems<'a, N>; + impl Graph { pub fn new() -> Graph { Graph { nodes: HashMap::new() } } pub fn add(&mut self, node: N, children: &[N]) { - self.nodes.insert(node, children.to_owned()); + self.nodes.insert(node, children.iter().map(|n| n.clone()).collect()); + } + + pub fn link(&mut self, node: N, child: N) { + self.nodes + .find_or_insert_with(node, |_| HashSet::new()) + .insert(child); + } + + pub fn edges<'a>(&'a self, node: &N) -> Option> { + self.nodes.find(node).map(|set| set.iter()) } pub fn sort(&self) -> Option> { @@ -44,4 +58,8 @@ impl Graph { dst.push(node.clone()); marks.insert(node.clone(), Done); } + + pub fn iter<'a>(&'a self) -> Nodes<'a, N> { + self.nodes.keys() + } } diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 12f86c7d9..71240d006 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -8,6 +8,7 @@ pub use self::paths::realpath; pub use self::hex::{to_hex, short_hash}; pub use self::pool::TaskPool; pub use self::dependency_queue::{DependencyQueue, Fresh, Dirty, Freshness}; +pub use self::graph::Graph; pub mod graph; pub mod process_builder; diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index 10f82fbb2..1c91da209 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -93,7 +93,7 @@ test!(cargo_compile_with_invalid_code { {filename}:1 invalid rust code! ^~~~~~~ Could not execute process \ -`rustc {filename} --crate-name foo --crate-type bin -g -o {} -L {} -L {}` (status=101)\n", +`rustc {filename} --crate-name foo --crate-type bin -o {} -L {} -L {}` (status=101)\n", target.join("foo").display(), target.display(), target.join("deps").display(), @@ -973,7 +973,7 @@ test!(verbose_build { let hash = out.slice_from(out.find_str("extra-filename=").unwrap() + 15); let hash = hash.slice_to(17); assert_eq!(out, format!("\ -{} `rustc {dir}{sep}src{sep}lib.rs --crate-name test --crate-type lib -g \ +{} `rustc {dir}{sep}src{sep}lib.rs --crate-name test --crate-type lib \ -C metadata=test:-:0.0.0:-:file:{dir} \ -C extra-filename={hash} \ --out-dir {dir}{sep}target \