From 848f8b5955dcc9b5c3d256cd5b6d891052075556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benno=20F=C3=BCnfst=C3=BCck?= Date: Fri, 1 Jul 2016 22:36:05 +0200 Subject: [PATCH] Disallow specifying features of transitive deps Before this commit, it was possible to activate a feature in a transtive dependency, using a Cargo.toml like the following one: ... [features] # this will enable feature fast in package bar, which is a # dependency of foo default = [ foo/bar/fast ] This is a bug, and was never intended, and it is checked in other places already. The behavior was possible because `build_features::add_feature` treats the specification "foo/bar/fast" as just another feature. So when we require the feature "foo/bar/fast", add_feature for foo will generate a dependency on "foo" requiring that feature "bar/fast" is enabled. Then, when resolving foo, add_feature will find that "bar/fast" is a required feature, so it'll happily add "fast" as the required feature for the dependency "foo". The fix for this is to make sure that the `add_feature` function does not treat `a/b` specifications as just another feature. Instead, it now handles that case without recursion directly when it encounters it. We can see how this resolves the above problem: when resolving foo, add_feature for the required feature "bar/fast" will be called. Because add_feature no longer treats such specifciations differently at the top level, it will try to enable a feature with the exact name "bar/fast", and Context::resolve_features will later find that no such feature exists for package foo. To give a friendlier error message, we also check in Context::resolve_features that we never ever require a feature with a slash in a name from a dependency. --- src/cargo/core/resolver/mod.rs | 79 +++++++++++++++++++--------------- tests/features.rs | 53 ++++++++++++++++++++++- 2 files changed, 97 insertions(+), 35 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index be690ba4305..4ce37c1e417 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -658,41 +658,53 @@ fn build_features(s: &Summary, method: &Method) visited: &mut HashSet) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } - // If this feature is of the form `foo/bar`, then we just lookup package - // `foo` and enable its feature `bar`. Otherwise this feature is of the - // form `foo` and we need to recurse to enable the feature `foo` for our - // own package, which may end up enabling more features or just enabling - // a dependency. - let mut parts = feat.splitn(2, '/'); - let feat_or_package = parts.next().unwrap(); - match parts.next() { - Some(feat) => { - let package = feat_or_package; - used.insert(package.to_string()); - deps.entry(package.to_string()) - .or_insert(Vec::new()) - .push(feat.to_string()); - } - None => { - let feat = feat_or_package; - if !visited.insert(feat.to_string()) { - bail!("Cyclic feature dependency: feature `{}` depends \ - on itself", feat) - } - used.insert(feat.to_string()); - match s.features().get(feat) { - Some(recursive) => { - for f in recursive { - try!(add_feature(s, f, deps, used, visited)); + if !visited.insert(feat.to_string()) { + bail!("Cyclic feature dependency: feature `{}` depends \ + on itself", feat) + } + + used.insert(feat.to_string()); + + // The lookup of here may fail if the feature corresponds to an optional + // dependency. If that is the case, we simply enable the optional dependency. + // + // Otherwise, we check what other features the feature wants and recursively + // add them. + match s.features().get(feat) { + Some(wanted_features) => { + for entry in wanted_features { + // If the entry is of the form `foo/bar`, then we just lookup package + // `foo` and enable its feature `bar`. We also add `foo` to the used + // set because `foo` might have been an optional dependency. + // + // Otherwise the entry refers to another feature of our current package, + // so we recurse by calling add_feature again, which may end up enabling + // more features or just enabling a dependency (remember, optional + // dependencies create an implicit feature with the same name). + let mut parts = entry.splitn(2, '/'); + let feat_or_package = parts.next().unwrap(); + match parts.next() { + Some(feat) => { + let package = feat_or_package; + used.insert(package.to_string()); + deps.entry(package.to_string()) + .or_insert(Vec::new()) + .push(feat.to_string()); + } + None => { + let feat = feat_or_package; + try!(add_feature(s, feat, deps, used, visited)); } - } - None => { - deps.entry(feat.to_string()).or_insert(Vec::new()); } } - visited.remove(&feat.to_string()); + } + None => { + deps.entry(feat.to_string()).or_insert(Vec::new()); } } + + visited.remove(&feat.to_string()); + Ok(()) } } @@ -843,11 +855,10 @@ impl<'a> Context<'a> { continue } let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]); - for feature in dep.features().iter() { - base.push(feature.clone()); + base.extend(dep.features().iter().map(|x| x.clone())); + for feature in base.iter() { if feature.contains("/") { - bail!("features in dependencies cannot enable features in \ - other dependencies: `{}`", feature) + bail!("feature names may not contain slashes: `{}`", feature); } } ret.push((dep.clone(), base)); diff --git a/tests/features.rs b/tests/features.rs index 5fcefaae6bb..4fea2735846 100644 --- a/tests/features.rs +++ b/tests/features.rs @@ -220,7 +220,58 @@ fn invalid8() { assert_that(p.cargo_process("build").arg("--features").arg("foo"), execs().with_status(101).with_stderr("\ -[ERROR] features in dependencies cannot enable features in other dependencies: `foo/bar` +[ERROR] feature names may not contain slashes: `foo/bar` +")); +} + +#[test] +fn no_transitive_dep_feature_requirement() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies.derived] + path = "derived" + + [features] + default = ["derived/bar/qux"] + "#) + .file("src/main.rs", r#" + extern crate derived; + fn main() { derived::test(); } + "#) + .file("derived/Cargo.toml", r#" + [package] + name = "derived" + version = "0.0.1" + authors = [] + + [dependencies.bar] + path = "../bar" + "#) + .file("derived/src/lib.rs", r#" + extern crate bar; + pub use bar::test; + "#) + .file("bar/Cargo.toml", r#" + [package] + name = "bar" + version = "0.0.1" + authors = [] + + [features] + qux = [] + "#) + .file("bar/src/lib.rs", r#" + #[cfg(feature = "qux")] + pub fn test() { print!("test"); } + "#); + assert_that(p.cargo_process("build"), + execs().with_status(101).with_stderr("\ +[ERROR] feature names may not contain slashes: `bar/qux` ")); }