Allow reexporting of features between packages

As pointed in #633, it's currently not possible for a package to reexport the
feature of another package due to the limitations of how features are defined.

This commit adds support for this ability by allowing features of the form
`foo/bar` in the `features` section of the manifest. This form indicates that
the dependency `foo` should have its `bar` feature enabled. Additionally, it is
not required that `foo` is an optional dependency.

This does not allow features of the form `foo/bar` in a `[dependencies]`
features section as dependencies shouldn't be enabling features for other
dependencies.

Closes #633
Closes #674

Author: alexcrichton

src/cargo/core/resolver.rs

@@ -318,40 +318,7 @@ fn resolve_deps<'a, R: Registry>(parent: &Summary,
                                  method: ResolveMethod,
                                  ctx: &mut Context<'a, R>)
                                  -> CargoResult<()> {
-    let dev_deps = match method {
-        ResolveEverything => true,
-        ResolveRequired(dev_deps, _, _) => dev_deps,
-    };
-
-    // First, filter by dev-dependencies
-    let deps = parent.get_dependencies();
-    let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
-
-    // Second, weed out optional dependencies, but not those required
-    let (mut feature_deps, used_features) = try!(build_features(parent, method));
-    let deps = deps.filter(|d| {
-        !d.is_optional() || feature_deps.remove(&d.get_name().to_string())
-    }).collect::<Vec<&Dependency>>();
-
-    // All features can only point to optional dependencies, in which case they
-    // should have all been weeded out by the above iteration. Any remaining
-    // features are bugs in that the package does not actually have those
-    // features.
-    if feature_deps.len() > 0 {
-        let features = feature_deps.iter().map(|s| s.as_slice())
-                                   .collect::<Vec<&str>>().connect(", ");
-        return Err(human(format!("Package `{}` does not have these features: \
-                                  `{}`", parent.get_package_id(), features)))
-    }
-
-    // Record what list of features is active for this package.
-    {
-        let pkgid = parent.get_package_id().clone();
-        match ctx.resolve.features.entry(pkgid) {
-            Occupied(entry) => entry.into_mut(),
-            Vacant(entry) => entry.set(HashSet::new()),
-        }.extend(used_features.into_iter());
-    }
+    let (deps, features) = try!(resolve_features(parent, method, ctx));
 
     // Recursively resolve all dependencies
     for &dep in deps.iter() {
@@ -409,8 +376,15 @@ fn resolve_deps<'a, R: Registry>(parent: &Summary,
                                       depends on itself",
                                      summary.get_package_id())))
         }
+
+        // The list of enabled features for this dependency are both those
+        // listed in the dependency itself as well as any of our own features
+        // which enabled a feature of this package.
+        let features = features.find_equiv(&dep.get_name())
+                               .map(|v| v.as_slice())
+                               .unwrap_or(&[]);
         try!(resolve_deps(summary,
-                          ResolveRequired(false, dep.get_features(),
+                          ResolveRequired(false, features,
                                           dep.uses_default_features()),
                           ctx));
         if dep.is_transitive() {
@@ -421,10 +395,81 @@ fn resolve_deps<'a, R: Registry>(parent: &Summary,
     Ok(())
 }
 
+fn resolve_features<'a, R>(parent: &'a Summary, method: ResolveMethod,
+                           ctx: &mut Context<R>)
+                           -> CargoResult<(Vec<&'a Dependency>,
+                                           HashMap<&'a str, Vec<String>>)> {
+    let dev_deps = match method {
+        ResolveEverything => true,
+        ResolveRequired(dev_deps, _, _) => dev_deps,
+    };
+
+    // First, filter by dev-dependencies
+    let deps = parent.get_dependencies();
+    let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
+
+    // Second, weed out optional dependencies, but not those required
+    let (mut feature_deps, used_features) = try!(build_features(parent, method));
+    let mut ret = HashMap::new();
+    let deps = deps.filter(|d| {
+        !d.is_optional() || feature_deps.contains_key_equiv(&d.get_name())
+    }).collect::<Vec<&Dependency>>();
+
+    // Next, sanitize all requested features by whitelisting all the requested
+    // features that correspond to optional dependencies
+    for dep in deps.iter() {
+        let mut base = feature_deps.pop_equiv(&dep.get_name())
+                                   .unwrap_or(Vec::new());
+        for feature in dep.get_features().iter() {
+            base.push(feature.clone());
+            if feature.as_slice().contains("/") {
+                return Err(human(format!("features in dependencies \
+                                          cannot enable features in \
+                                          other dependencies: `{}`",
+                                         feature)));
+            }
+        }
+        ret.insert(dep.get_name(), base);
+    }
+
+    // All features can only point to optional dependencies, in which case they
+    // should have all been weeded out by the above iteration. Any remaining
+    // features are bugs in that the package does not actually have those
+    // features.
+    if feature_deps.len() > 0 {
+        let unknown = feature_deps.keys().map(|s| s.as_slice())
+                                  .collect::<Vec<&str>>();
+        if unknown.len() > 0 {
+            let features = unknown.connect(", ");
+            return Err(human(format!("Package `{}` does not have these features: \
+                                      `{}`", parent.get_package_id(), features)))
+        }
+    }
+
+    // Record what list of features is active for this package.
+    {
+        let pkgid = parent.get_package_id().clone();
+        match ctx.resolve.features.entry(pkgid) {
+            Occupied(entry) => entry.into_mut(),
+            Vacant(entry) => entry.set(HashSet::new()),
+        }.extend(used_features.into_iter());
+    }
+
+    Ok((deps, ret))
+}
+
 // Returns a pair of (feature dependencies, all used features)
+//
+// The feature dependencies map is a mapping of package name to list of features
+// enabled. Each package should be enabled, and each package should have the
+// specified set of features enabled.
+//
+// The all used features set is the set of features which this local package had
+// enabled, which is later used when compiling to instruct the code what
+// features were enabled.
 fn build_features(s: &Summary, method: ResolveMethod)
-                  -> CargoResult<(HashSet<String>, HashSet<String>)> {
-    let mut deps = HashSet::new();
+                  -> CargoResult<(HashMap<String, Vec<String>>, HashSet<String>)> {
+    let mut deps = HashMap::new();
     let mut used = HashSet::new();
     let mut visited = HashSet::new();
     match method {
@@ -454,26 +499,51 @@ fn build_features(s: &Summary, method: ResolveMethod)
     return Ok((deps, used));
 
     fn add_feature(s: &Summary, feat: &str,
-                   deps: &mut HashSet<String>,
+                   deps: &mut HashMap<String, Vec<String>>,
                    used: &mut HashSet<String>,
                    visited: &mut HashSet<String>) -> CargoResult<()> {
-        if feat.is_empty() {
-            return Ok(())
-        }
-        if !visited.insert(feat.to_string()) {
-            return Err(human(format!("Cyclic feature dependency: feature `{}` \
-                                      depends on itself", feat)))
-        }
-        used.insert(feat.to_string());
-        match s.get_features().find_equiv(&feat) {
-            Some(recursive) => {
-                for f in recursive.iter() {
-                    try!(add_feature(s, f.as_slice(), deps, used, visited));
+        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(1, '/');
+        let feat_or_package = parts.next().unwrap();
+        match parts.next() {
+            Some(feat) => {
+                let package = feat_or_package;
+                match deps.entry(package.to_string()) {
+                    Occupied(e) => e.into_mut(),
+                    Vacant(e) => e.set(Vec::new()),
+                }.push(feat.to_string());
+            }
+            None => {
+                let feat = feat_or_package;
+                if !visited.insert(feat.to_string()) {
+                    return Err(human(format!("Cyclic feature dependency: \
+                                              feature `{}` depends on itself",
+                                              feat)))
+                }
+                used.insert(feat.to_string());
+                match s.get_features().find_equiv(&feat) {
+                    Some(recursive) => {
+                        for f in recursive.iter() {
+                            try!(add_feature(s, f.as_slice(), deps, used,
+                                             visited));
+                        }
+                    }
+                    None => {
+                        match deps.entry(feat.to_string()) {
+                            Occupied(..) => {} // already activated
+                            Vacant(e) => { e.set(Vec::new()); }
+                        }
+                    }
                 }
+                visited.remove(&feat.to_string());
             }
-            None => { deps.insert(feat.to_string()); }
         }
-        visited.remove(&feat.to_string());
         Ok(())
     }
 }

src/cargo/core/summary.rs

@@ -30,19 +30,24 @@ impl Summary {
         }
         for (feature, list) in features.iter() {
             for dep in list.iter() {
-                if features.find_equiv(&dep.as_slice()).is_some() { continue }
-                let d = dependencies.iter().find(|d| {
-                    d.get_name() == dep.as_slice()
-                });
-                match d {
+                let mut parts = dep.as_slice().splitn(1, '/');
+                let dep = parts.next().unwrap();
+                let is_reexport = parts.next().is_some();
+                if !is_reexport && features.find_equiv(&dep).is_some() { continue }
+                match dependencies.iter().find(|d| d.get_name() == dep) {
                     Some(d) => {
-                        if d.is_optional() { continue }
+                        if d.is_optional() || is_reexport { continue }
                         return Err(human(format!("Feature `{}` depends on `{}` \
                                                   which is not an optional \
                                                   dependency.\nConsider adding \
                                                   `optional = true` to the \
                                                   dependency", feature, dep)))
                     }
+                    None if is_reexport => {
+                        return Err(human(format!("Feature `{}` requires `{}` \
+                                                  which is not an optional \
+                                                  dependency", feature, dep)))
+                    }
                     None => {
                         return Err(human(format!("Feature `{}` includes `{}` \
                                                   which is neither a dependency \

src/doc/manifest.md

@@ -157,6 +157,11 @@ default = ["jquery", "uglifier"]
 # requirements to the feature in the future.
 secure-password = ["bcrypt"]
 
+# Features can be used to reexport features of other packages.
+# The `session` feature of package `awesome` will ensure that the
+# `session` feature of the package `cookie` is also enabled.
+session = ["cookie/session"]
+
 [dependencies]
 
 # These packages are mandatory and form the core of this

src/snapshots.txt

@@ -1,3 +1,11 @@
+2014-10-16
+  linux-i386 61417861716cd41d8f372be36bb0572e4f29dec8
+  linux-x86_64 59be4ff9f547f1ba47ad133ab74151a48bc2659b
+  macos-i386 cb5267d2e7df8406c26bb0337b1c2e80b125e2cb
+  macos-x86_64 9283adb4dfd1b60c7bfe38ef755f9187fe7d5580
+  winnt-i386 88deb2950fa2b73358bc15763e6373ade6325f53
+  winnt-x86_64 0143d4b0e4b20e84dbb27a4440b4b55d369f4456
+
 2014-09-19
   linux-i386 c92895421e6fa170dbd713e74334b8c3cf22b817
   linux-x86_64 66ee4126f9e4820cd82e78181931f8ea365904de

tests/test_cargo_features.rs

@@ -137,6 +137,76 @@ Dev-dependencies are not allowed to be optional: `bar`
 ").as_slice()));
 })
 
+test!(invalid6 {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [features]
+            foo = ["bar/baz"]
+        "#)
+        .file("src/main.rs", "");
+
+    assert_that(p.cargo_process("build").arg("--features").arg("foo"),
+                execs().with_status(101).with_stderr(format!("\
+Cargo.toml is not a valid manifest
+
+Feature `foo` requires `bar` which is not an optional dependency
+").as_slice()));
+})
+
+test!(invalid7 {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [features]
+            foo = ["bar/baz"]
+            bar = []
+        "#)
+        .file("src/main.rs", "");
+
+    assert_that(p.cargo_process("build").arg("--features").arg("foo"),
+                execs().with_status(101).with_stderr(format!("\
+Cargo.toml is not a valid manifest
+
+Feature `foo` requires `bar` which is not an optional dependency
+").as_slice()));
+})
+
+test!(invalid8 {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.bar]
+            path = "bar"
+            features = ["foo/bar"]
+        "#)
+        .file("src/main.rs", "")
+        .file("bar/Cargo.toml", r#"
+            [package]
+            name = "bar"
+            version = "0.0.1"
+            authors = []
+        "#)
+        .file("bar/src/lib.rs", "");
+
+    assert_that(p.cargo_process("build").arg("--features").arg("foo"),
+                execs().with_status(101).with_stderr(format!("\
+features in dependencies cannot enable features in other dependencies: `foo/bar`
+").as_slice()));
+})
+
 test!(no_feature_doesnt_build {
     let p = project("foo")
         .file("Cargo.toml", r#"
@@ -477,3 +547,40 @@ test!(empty_features {
     assert_that(p.cargo_process("build").arg("--features").arg(""),
                 execs().with_status(0));
 })
+
+// Tests that all cmd lines work with `--features ""`
+test!(transitive_features {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [features]
+            foo = ["bar/baz"]
+
+            [dependencies.bar]
+            path = "bar"
+        "#)
+        .file("src/main.rs", "
+            extern crate bar;
+            fn main() { bar::baz(); }
+        ")
+        .file("bar/Cargo.toml", r#"
+            [package]
+            name = "bar"
+            version = "0.0.1"
+            authors = []
+
+            [features]
+            baz = []
+        "#)
+        .file("bar/src/lib.rs", r#"
+            #[cfg(feature = "baz")]
+            pub fn baz() {}
+        "#);
+
+    assert_that(p.cargo_process("build").arg("--features").arg("foo"),
+                execs().with_status(0));
+})