From 25644b7f51cedd669e7ac864746b78268be8d84d Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 17 Jul 2015 10:05:17 -0700 Subject: [PATCH 1/2] Remove an unnecessary HashMap when Vec works Dependencies aren't necessarily unique per name as the same dependency can show up multiple times in various dependency kind lists, so it's not quite right to use a hash map anyway. --- src/cargo/core/resolver/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index a72d7a53414..7b04e93aeaf 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -182,7 +182,7 @@ fn activate(mut cx: Box, // Next, transform all dependencies into a list of possible candidates which // can satisfy that dependency. - let mut deps = try!(deps.into_iter().map(|(_dep_name, (dep, features))| { + let mut deps = try!(deps.into_iter().map(|(dep, features)| { let mut candidates = try!(registry.query(dep)); // When we attempt versions for a package, we'll want to start at the // maximum version and work our way down. @@ -430,8 +430,7 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool { fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary, method: Method) - -> CargoResult)>> { + -> CargoResult)>> { let dev_deps = match method { Method::Everything => true, Method::Required { dev_deps, .. } => dev_deps, @@ -452,7 +451,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary, }); let (mut feature_deps, used_features) = try!(build_features(parent, method)); - let mut ret = HashMap::new(); + let mut ret = Vec::new(); // Next, sanitize all requested features by whitelisting all the requested // features that correspond to optional dependencies @@ -461,7 +460,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary, if dep.is_optional() && !feature_deps.contains_key(dep.name()) { continue } - let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]); + let mut base = feature_deps.remove(dep.name()).unwrap_or(Vec::new()); for feature in dep.features().iter() { base.push(feature.clone()); if feature.contains("/") { @@ -471,7 +470,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary, feature))); } } - ret.insert(dep.name(), (dep, base)); + ret.push((dep, base)); } // All features can only point to optional dependencies, in which case they From 70152d800313e0db1c4112e0debed960e058335e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 17 Jul 2015 10:05:57 -0700 Subject: [PATCH 2/2] Only compile activated optional dependencies Development dependencies are traversed during the resolution process, causing them to be returned as the list of dependencies for a package in terms of resolution. The compilation phase would then filter these out depending on the dependency's transitivity, but this was incorrectly accounted for when the dependency showed up multiple times in a few lists. This commit fixes this behavior by updating the filtering logic to ensure that only activated optional dependencies (those whose feature names are listed) are compiled. Closes #1805 --- src/cargo/ops/cargo_rustc/context.rs | 10 ++++++++-- tests/test_cargo_features.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index 26b4a14c079..2782213ddf3 100644 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -351,7 +351,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> { pkg.dependencies().iter().filter(|d| { d.name() == dep.name() }).any(|d| { - // If this target is a build command, then we only want build // dependencies, otherwise we want everything *other than* build // dependencies. @@ -364,7 +363,14 @@ impl<'a, 'cfg> Context<'a, 'cfg> { target.is_example() || profile.test; - is_correct_dep && is_actual_dep + // If the dependency is optional, then we're only activating it + // if the corresponding feature was activated + let activated = !d.is_optional() || + self.resolve.features(pkg.package_id()).map(|f| { + f.contains(d.name()) + }).unwrap_or(false); + + is_correct_dep && is_actual_dep && activated }) }).filter_map(|pkg| { pkg.targets().iter().find(|t| t.is_lib()).map(|t| { diff --git a/tests/test_cargo_features.rs b/tests/test_cargo_features.rs index 3d86dcedf89..33a058f0e32 100644 --- a/tests/test_cargo_features.rs +++ b/tests/test_cargo_features.rs @@ -741,3 +741,31 @@ test!(unions_work_with_no_default_features { assert_that(p.cargo("build"), execs().with_status(0).with_stdout("")); assert_that(p.cargo("build"), execs().with_status(0).with_stdout("")); }); + +test!(optional_and_dev_dep { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "test" + version = "0.1.0" + authors = [] + + [dependencies] + foo = { path = "foo", optional = true } + [dev-dependencies] + foo = { path = "foo" } + "#) + .file("src/lib.rs", "") + .file("foo/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("foo/src/lib.rs", ""); + + assert_that(p.cargo_process("build"), + execs().with_status(0).with_stdout(format!("\ +{compiling} test v0.1.0 ([..]) +", compiling = COMPILING))); +});