Auto merge of #3280 - alexcrichton:test-features, r=brson

Fix passing --features when testing multiple packages

The wrong method was being passed to resolution accidentally. Features specified
via `--features` and `--no-default-features` are spec'd as only applying to the
*current* package, not all packages.

Closes #3279

Author: bors

src/cargo/ops/cargo_compile.rs

@@ -103,7 +103,7 @@ pub fn resolve_dependencies<'a>(ws: &Workspace<'a>,
                                 features: &[String],
                                 all_features: bool,
                                 no_default_features: bool,
-                                spec: &'a [String])
+                                specs: &[PackageIdSpec])
                                 -> CargoResult<(PackageSet<'a>, Resolve)> {
     let features = features.iter().flat_map(|s| {
         s.split_whitespace()
@@ -137,13 +137,10 @@ pub fn resolve_dependencies<'a>(ws: &Workspace<'a>,
         }
     };
 
-    let specs = try!(spec.iter().map(|p| PackageIdSpec::parse(p))
-                                .collect::<CargoResult<Vec<_>>>());
-
     let resolved_with_overrides =
             try!(ops::resolve_with_previous(&mut registry, ws,
                                             method, Some(&resolve), None,
-                                            &specs));
+                                            specs));
 
     let packages = ops::get_resolved_packages(&resolved_with_overrides,
                                               registry);
@@ -174,8 +171,16 @@ pub fn compile_ws<'a>(ws: &Workspace<'a>,
         try!(generate_targets(root_package, profiles, mode, filter, release));
     }
 
-    let (packages, resolve_with_overrides) =
-        try!(resolve_dependencies(ws, source, features, all_features, no_default_features, spec));
+    let specs = try!(spec.iter().map(|p| PackageIdSpec::parse(p))
+                                .collect::<CargoResult<Vec<_>>>());
+
+    let pair = try!(resolve_dependencies(ws,
+                                         source,
+                                         features,
+                                         all_features,
+                                         no_default_features,
+                                         &specs));
+    let (packages, resolve_with_overrides) = pair;
 
     let mut pkgids = Vec::new();
     if spec.len() > 0 {

src/cargo/ops/cargo_output_metadata.rs

@@ -1,7 +1,7 @@
 use rustc_serialize::{Encodable, Encoder};
 
 use core::resolver::Resolve;
-use core::{Package, PackageId, Workspace};
+use core::{Package, PackageId, PackageIdSpec, Workspace};
 use ops;
 use util::CargoResult;
 
@@ -43,12 +43,15 @@ fn metadata_no_deps(ws: &Workspace,
 
 fn metadata_full(ws: &Workspace,
                  opt: &OutputMetadataOptions) -> CargoResult<ExportInfo> {
+    let specs = ws.members().map(|pkg| {
+        PackageIdSpec::from_package_id(pkg.package_id())
+    }).collect::<Vec<_>>();
     let deps = try!(ops::resolve_dependencies(ws,
                                               None,
                                               &opt.features,
                                               opt.all_features,
                                               opt.no_default_features,
-                                              &[]));
+                                              &specs));
     let (packages, resolve) = deps;
 
     let packages = try!(packages.package_ids()

src/cargo/ops/resolve.rs

@@ -90,24 +90,50 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
     for member in ws.members() {
         try!(registry.add_sources(&[member.package_id().source_id()
                                           .clone()]));
+        let method_to_resolve = match method {
+            // When everything for a workspace we want to be sure to resolve all
+            // members in the workspace, so propagate the `Method::Everything`.
+            Method::Everything => Method::Everything,
 
-        // If we're resolving everything then we include all members of the
-        // workspace. If we want a specific set of requirements and we're
-        // compiling only a single workspace crate then resolve only it. This
-        // case should only happen after we have a previous resolution, however,
-        // so assert that the previous exists.
-        if let Method::Required { .. } = method {
-            assert!(previous.is_some());
-            if let Some(current) = ws.current_opt() {
-                if member.package_id() != current.package_id() &&
-                   !specs.iter().any(|spec| spec.matches(member.package_id())) {
-                    continue;
+            // If we're not resolving everything though then the workspace is
+            // already resolved and now we're drilling down from that to the
+            // exact crate graph we're going to build. Here we don't necessarily
+            // want to keep around all workspace crates as they may not all be
+            // built/tested.
+            //
+            // Additionally, the `method` specified represents command line
+            // flags, which really only matters for the current package
+            // (determined by the cwd). If other packages are specified (via
+            // `-p`) then the command line flags like features don't apply to
+            // them.
+            //
+            // As a result, if this `member` is the current member of the
+            // workspace, then we use `method` specified. Otherwise we use a
+            // base method with no features specified but using default features
+            // for any other packages specified with `-p`.
+            Method::Required { dev_deps, .. } => {
+                assert!(previous.is_some());
+                let base = Method::Required {
+                    dev_deps: dev_deps,
+                    features: &[],
+                    uses_default_features: true,
+                };
+                let member_id = member.package_id();
+                match ws.current_opt() {
+                    Some(current) if member_id == current.package_id() => method,
+                    _ => {
+                        if specs.iter().any(|spec| spec.matches(member_id)) {
+                            base
+                        } else {
+                            continue
+                        }
+                    }
                 }
             }
-        }
+        };
 
         let summary = registry.lock(member.summary().clone());
-        summaries.push((summary, method));
+        summaries.push((summary, method_to_resolve));
     }
 
     let root_replace = ws.root_replace();

tests/test.rs

@@ -2364,3 +2364,37 @@ fn test_release_ignore_panic() {
     println!("bench");
     assert_that(p.cargo("bench").arg("-v"), execs().with_status(0));
 }
+
+#[test]
+fn test_many_with_features() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            a = { path = "a" }
+
+            [features]
+            foo = []
+
+            [workspace]
+        "#)
+        .file("src/lib.rs", "")
+        .file("a/Cargo.toml", r#"
+            [package]
+            name = "a"
+            version = "0.0.1"
+            authors = []
+        "#)
+        .file("a/src/lib.rs", "");
+    p.build();
+
+    assert_that(p.cargo("test").arg("-v")
+                 .arg("-p").arg("a")
+                 .arg("-p").arg("foo")
+                 .arg("--features").arg("foo"),
+                execs().with_status(0));
+}