diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index b503f1676b5..4b633c26d69 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -57,6 +57,7 @@ use url::Url; use core::{PackageId, Registry, SourceId, Summary, Dependency}; use core::PackageIdSpec; +use util::config::Config; use util::Graph; use util::errors::{CargoResult, CargoError}; use util::profile; @@ -330,6 +331,9 @@ struct Context<'a> { resolve_replacements: RcList<(PackageId, PackageId)>, replacements: &'a [(PackageIdSpec, Dependency)], + + // These warnings are printed after resolution. + warnings: RcList, } type Activations = HashMap>>; @@ -337,13 +341,15 @@ type Activations = HashMap>>; /// Builds the list of all packages required to build the first argument. pub fn resolve(summaries: &[(Summary, Method)], replacements: &[(PackageIdSpec, Dependency)], - registry: &mut Registry) -> CargoResult { + registry: &mut Registry, + config: Option<&Config>) -> CargoResult { let cx = Context { resolve_graph: RcList::new(), resolve_features: HashMap::new(), resolve_replacements: RcList::new(), activations: HashMap::new(), replacements: replacements, + warnings: RcList::new(), }; let _p = profile::start(format!("resolving")); let cx = activate_deps_loop(cx, registry, summaries)?; @@ -368,8 +374,18 @@ pub fn resolve(summaries: &[(Summary, Method)], } check_cycles(&resolve, &cx.activations)?; - trace!("resolved: {:?}", resolve); + + // If we have a shell, emit warnings about required deps used as feature. + if let Some(config) = config { + let mut shell = config.shell(); + let mut warnings = &cx.warnings; + while let Some(ref head) = warnings.head { + shell.warn(&head.0)?; + warnings = &head.1; + } + } + Ok(resolve) } @@ -827,13 +843,15 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool { // // 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. +// specified set of features enabled. The boolean indicates whether this +// package was specifically requested (rather than just requesting features +// *within* this package). // // 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<'a>(s: &'a Summary, method: &'a Method) - -> CargoResult<(HashMap<&'a str, Vec>, HashSet<&'a str>)> { + -> CargoResult<(HashMap<&'a str, (bool, Vec)>, HashSet<&'a str>)> { let mut deps = HashMap::new(); let mut used = HashSet::new(); let mut visited = HashSet::new(); @@ -867,7 +885,7 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) fn add_feature<'a>(s: &'a Summary, feat: &'a str, - deps: &mut HashMap<&'a str, Vec>, + deps: &mut HashMap<&'a str, (bool, Vec)>, used: &mut HashSet<&'a str>, visited: &mut HashSet<&'a str>) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } @@ -884,8 +902,8 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) let package = feat_or_package; used.insert(package); deps.entry(package) - .or_insert(Vec::new()) - .push(feat.to_string()); + .or_insert((false, Vec::new())) + .1.push(feat.to_string()); } None => { let feat = feat_or_package; @@ -896,12 +914,14 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) used.insert(feat); match s.features().get(feat) { Some(recursive) => { + // This is a feature, add it recursively. for f in recursive { add_feature(s, f, deps, used, visited)?; } } None => { - deps.entry(feat).or_insert(Vec::new()); + // This is a dependency, mark it as explicitly requested. + deps.entry(feat).or_insert((false, Vec::new())).0 = true; } } visited.remove(feat); @@ -1057,8 +1077,9 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } + /// Return all dependencies and the features we want from them. fn resolve_features<'b>(&mut self, - candidate: &'b Summary, + s: &'b Summary, method: &'b Method) -> CargoResult)>> { let dev_deps = match *method { @@ -1067,21 +1088,31 @@ impl<'a> Context<'a> { }; // First, filter by dev-dependencies - let deps = candidate.dependencies(); + let deps = s.dependencies(); let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps); - let (mut feature_deps, used_features) = build_features(candidate, - method)?; + let (mut feature_deps, used_features) = build_features(s, method)?; let mut ret = Vec::new(); - // Next, sanitize all requested features by whitelisting all the - // requested features that correspond to optional dependencies + // Next, collect all actually enabled dependencies and their features. for dep in deps { - // weed out optional dependencies, but not those required + // Skip optional dependencies, but not those enabled through a feature if dep.is_optional() && !feature_deps.contains_key(dep.name()) { continue } - let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]); + // So we want this dependency. Move the features we want from `feature_deps` + // to `ret`. + let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![])); + if !dep.is_optional() && base.0 { + self.warnings.push( + format!("Package `{}` does not have feature `{}`. It has a required dependency \ + with that name, but only optional dependencies can be used as features. \ + This is currently a warning to ease the transition, but it will become an \ + error in the future.", + s.package_id(), dep.name()) + ); + } + let mut base = base.1; base.extend(dep.features().iter().cloned()); for feature in base.iter() { if feature.contains("/") { @@ -1091,23 +1122,20 @@ impl<'a> Context<'a> { ret.push((dep.clone(), 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. + // Any remaining entries in feature_deps are bugs in that the package does not actually + // have those dependencies. We classified them as dependencies in the first place + // because there is no such feature, either. if !feature_deps.is_empty() { let unknown = feature_deps.keys().map(|s| &s[..]) .collect::>(); - if !unknown.is_empty() { - let features = unknown.join(", "); - bail!("Package `{}` does not have these features: `{}`", - candidate.package_id(), features) - } + let features = unknown.join(", "); + bail!("Package `{}` does not have these features: `{}`", + s.package_id(), features) } // Record what list of features is active for this package. if !used_features.is_empty() { - let pkgid = candidate.package_id(); + let pkgid = s.package_id(); let set = self.resolve_features.entry(pkgid.clone()) .or_insert_with(HashSet::new); diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 9fdc326b8e5..9902311e66b 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -53,8 +53,8 @@ impl Summary { feature, dep) } None if is_reexport => { - bail!("Feature `{}` requires `{}` which is not an \ - optional dependency", feature, dep) + bail!("Feature `{}` requires a feature of `{}` which is not a \ + dependency", feature, dep) } None => { bail!("Feature `{}` includes `{}` which is neither \ diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 510f3d1c65a..29c3749c62c 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -19,7 +19,7 @@ pub fn generate_lockfile(ws: &Workspace) -> CargoResult<()> { let mut registry = PackageRegistry::new(ws.config())?; let resolve = ops::resolve_with_previous(&mut registry, ws, Method::Everything, - None, None, &[])?; + None, None, &[], true)?; ops::write_pkg_lockfile(ws, &resolve)?; Ok(()) } @@ -79,7 +79,8 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions) Method::Everything, Some(&previous_resolve), Some(&to_avoid), - &[])?; + &[], + true)?; // Summarize what is changing for the user. let print_change = |status: &str, msg: String| { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index d4387e49a38..cd88738cae4 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -15,7 +15,7 @@ use util::errors::{CargoResult, CargoResultExt}; /// lockfile. pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolve)> { let mut registry = PackageRegistry::new(ws.config())?; - let resolve = resolve_with_registry(ws, &mut registry)?; + let resolve = resolve_with_registry(ws, &mut registry, true)?; let packages = get_resolved_packages(&resolve, registry); Ok((packages, resolve)) } @@ -44,7 +44,7 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, let resolve = if ws.require_optional_deps() { // First, resolve the root_package's *listed* dependencies, as well as // downloading and updating all remotes and such. - let resolve = resolve_with_registry(ws, &mut registry)?; + let resolve = resolve_with_registry(ws, &mut registry, false)?; // Second, resolve with precisely what we're doing. Filter out // transitive dependencies if necessary, specify features, handle @@ -79,19 +79,19 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, let resolved_with_overrides = ops::resolve_with_previous(&mut registry, ws, method, resolve.as_ref(), None, - specs)?; + specs, true)?; let packages = get_resolved_packages(&resolved_with_overrides, registry); Ok((packages, resolved_with_overrides)) } -fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry) +fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry, warn: bool) -> CargoResult { let prev = ops::load_pkg_lockfile(ws)?; let resolve = resolve_with_previous(registry, ws, Method::Everything, - prev.as_ref(), None, &[])?; + prev.as_ref(), None, &[], warn)?; if !ws.is_ephemeral() { ops::write_pkg_lockfile(ws, &resolve)?; @@ -114,7 +114,8 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, method: Method, previous: Option<&'a Resolve>, to_avoid: Option<&HashSet<&'a PackageId>>, - specs: &[PackageIdSpec]) + specs: &[PackageIdSpec], + warn: bool) -> CargoResult { // Here we place an artificial limitation that all non-registry sources // cannot be locked at more than one revision. This means that if a git @@ -256,9 +257,15 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, None => root_replace.to_vec(), }; + let config = if warn { + Some(ws.config()) + } else { + None + }; let mut resolved = resolver::resolve(&summaries, &replace, - registry)?; + registry, + config)?; resolved.register_used_patches(registry.patches()); if let Some(previous) = previous { resolved.merge_from(previous)?; diff --git a/tests/features.rs b/tests/features.rs index 786db0470e4..7aec1b99e06 100644 --- a/tests/features.rs +++ b/tests/features.rs @@ -169,7 +169,7 @@ fn invalid6() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `foo` requires `bar` which is not an optional dependency + Feature `foo` requires a feature of `bar` which is not a dependency ")); } @@ -193,7 +193,7 @@ fn invalid7() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `foo` requires `bar` which is not an optional dependency + Feature `foo` requires a feature of `bar` which is not a dependency ")); } @@ -225,6 +225,80 @@ fn invalid8() { ")); } +#[test] +fn invalid9() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies.bar] + path = "bar" + "#) + .file("src/main.rs", "fn main() {}") + .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("bar"), + execs().with_status(0).with_stderr("\ +warning: Package `foo v0.0.1 ([..])` does not have feature `bar`. It has a required dependency with \ +that name, but only optional dependencies can be used as features. [..] + Compiling bar v0.0.1 ([..]) + Compiling foo v0.0.1 ([..]) + Finished dev [unoptimized + debuginfo] target(s) in [..] secs +")); +} + +#[test] +fn invalid10() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies.bar] + path = "bar" + features = ["baz"] + "#) + .file("src/main.rs", "fn main() {}") + .file("bar/Cargo.toml", r#" + [package] + name = "bar" + version = "0.0.1" + authors = [] + + [dependencies.baz] + path = "baz" + "#) + .file("bar/src/lib.rs", "") + .file("bar/baz/Cargo.toml", r#" + [package] + name = "baz" + version = "0.0.1" + authors = [] + "#) + .file("bar/baz/src/lib.rs", ""); + + assert_that(p.cargo_process("build"), + execs().with_status(0).with_stderr("\ +warning: Package `bar v0.0.1 ([..])` does not have feature `baz`. It has a required dependency with \ +that name, but only optional dependencies can be used as features. [..] + Compiling baz v0.0.1 ([..]) + Compiling bar v0.0.1 ([..]) + Compiling foo v0.0.1 ([..]) + Finished dev [unoptimized + debuginfo] target(s) in [..] secs +")); +} + #[test] fn no_transitive_dep_feature_requirement() { let p = project("foo") diff --git a/tests/resolve.rs b/tests/resolve.rs index 16395a25567..9b8ccb0a2eb 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -32,7 +32,7 @@ fn resolve(pkg: PackageId, deps: Vec, registry: &[Summary]) let mut registry = MyRegistry(registry); let summary = Summary::new(pkg.clone(), deps, HashMap::new()).unwrap(); let method = Method::Everything; - let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry)?; + let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None)?; let res = resolve.iter().cloned().collect(); Ok(res) }