From d21cb9a86a38a9d935b99c6f03c6fd116760a07e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 10 Nov 2016 15:44:39 -0800 Subject: [PATCH] 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 --- src/cargo/ops/cargo_compile.rs | 19 ++++++---- src/cargo/ops/cargo_output_metadata.rs | 7 +++- src/cargo/ops/resolve.rs | 52 +++++++++++++++++++------- tests/test.rs | 34 +++++++++++++++++ 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index bec83524419..4c385c8d169 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/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::>>()); - 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::>>()); + + 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 { diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index cda9b53e67f..affaaa6a18e 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/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 { + let specs = ws.members().map(|pkg| { + PackageIdSpec::from_package_id(pkg.package_id()) + }).collect::>(); 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() diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 0bde351e287..87936d3f91d 100644 --- a/src/cargo/ops/resolve.rs +++ b/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(); diff --git a/tests/test.rs b/tests/test.rs index b3ca52f8ce9..31b815385e3 100644 --- a/tests/test.rs +++ b/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)); +}