From c72df674d1c7d24015d84b70aa232684622d2c3f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Aug 2017 20:21:38 -0700 Subject: [PATCH 1/5] test for mandatory dependencies not to be considered features --- tests/features.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/features.rs b/tests/features.rs index 786db0470e4..f004d366aca 100644 --- a/tests/features.rs +++ b/tests/features.rs @@ -225,6 +225,71 @@ 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", "") + .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(101).with_stderr("\ +[ERROR] Package `foo v0.0.1 ([..])` does not have these features: `bar` +")); +} + +#[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", "") + .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(101).with_stderr("\ +[ERROR] Package `bar v0.0.1 ([..])` does not have these features: `baz` +")); +} + #[test] fn no_transitive_dep_feature_requirement() { let p = project("foo") From 396033472d3baec15e3d7242072655aa737d1fbd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Aug 2017 21:21:09 -0700 Subject: [PATCH 2/5] detect required dependencies used as features --- src/cargo/core/resolver/mod.rs | 56 +++++++++++++++++++--------------- tests/features.rs | 6 ++-- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index b503f1676b5..80ff03050c3 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -827,13 +827,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 +869,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 +886,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 +898,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 +1061,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 +1072,27 @@ 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 { + bail!("Package `{}` does not have feature `{}`. It has a required dependency with \ + that name, but only optional dependencies can be used as features.", + 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 +1102,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/tests/features.rs b/tests/features.rs index f004d366aca..629bea34a63 100644 --- a/tests/features.rs +++ b/tests/features.rs @@ -248,7 +248,8 @@ fn invalid9() { assert_that(p.cargo_process("build").arg("--features").arg("bar"), execs().with_status(101).with_stderr("\ -[ERROR] Package `foo v0.0.1 ([..])` does not have these features: `bar` +[ERROR] 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. ")); } @@ -286,7 +287,8 @@ fn invalid10() { assert_that(p.cargo_process("build"), execs().with_status(101).with_stderr("\ -[ERROR] Package `bar v0.0.1 ([..])` does not have these features: `baz` +[ERROR] 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. ")); } From 5cab69c207f61326bfc37af0896f8291c178ae53 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Aug 2017 21:27:26 -0700 Subject: [PATCH 3/5] improve misleading error message --- src/cargo/core/summary.rs | 4 ++-- tests/features.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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/tests/features.rs b/tests/features.rs index 629bea34a63..44cbf961032 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 ")); } From f942d8d4313ae63240e1155be0cac40ec3ba34ba Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 18 Aug 2017 19:37:12 +0200 Subject: [PATCH 4/5] Using required dep as a feature is a warning for now, not an error --- src/cargo/core/resolver/mod.rs | 29 ++++++++++++++++++++---- src/cargo/ops/cargo_generate_lockfile.rs | 5 ++-- src/cargo/ops/resolve.rs | 23 +++++++++++++------ tests/features.rs | 23 ++++++++++++------- tests/resolve.rs | 2 +- 5 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 80ff03050c3..2b751d6a890 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 core::shell::Shell; 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, + shell: Option<&mut Shell>) -> 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,17 @@ 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(shell) = shell { + let mut warnings = &cx.warnings; + while let Some(ref head) = warnings.head { + shell.warn(&head.0)?; + warnings = &head.1; + } + } + Ok(resolve) } @@ -1088,9 +1103,13 @@ impl<'a> Context<'a> { // to `ret`. let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![])); if !dep.is_optional() && base.0 { - bail!("Package `{}` does not have feature `{}`. It has a required dependency with \ - that name, but only optional dependencies can be used as features.", - s.package_id(), dep.name()); + 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()); 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..074f798985a 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,17 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, None => root_replace.to_vec(), }; + let mut shell; + let opt_shell = if warn { + shell = ws.config().shell(); + Some(&mut *shell) + } else { + None + }; let mut resolved = resolver::resolve(&summaries, &replace, - registry)?; + registry, + opt_shell)?; 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 44cbf961032..7aec1b99e06 100644 --- a/tests/features.rs +++ b/tests/features.rs @@ -237,7 +237,7 @@ fn invalid9() { [dependencies.bar] path = "bar" "#) - .file("src/main.rs", "") + .file("src/main.rs", "fn main() {}") .file("bar/Cargo.toml", r#" [package] name = "bar" @@ -247,9 +247,12 @@ fn invalid9() { .file("bar/src/lib.rs", ""); assert_that(p.cargo_process("build").arg("--features").arg("bar"), - execs().with_status(101).with_stderr("\ -[ERROR] 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. + 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 ")); } @@ -266,7 +269,7 @@ fn invalid10() { path = "bar" features = ["baz"] "#) - .file("src/main.rs", "") + .file("src/main.rs", "fn main() {}") .file("bar/Cargo.toml", r#" [package] name = "bar" @@ -286,9 +289,13 @@ fn invalid10() { .file("bar/baz/src/lib.rs", ""); assert_that(p.cargo_process("build"), - execs().with_status(101).with_stderr("\ -[ERROR] 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. + 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 ")); } 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) } From 8a4a291dd7cc4f8f37d877a79be962ec029d9b53 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 18 Aug 2017 20:10:34 +0200 Subject: [PATCH 5/5] fix borrowing the shell twice --- src/cargo/core/resolver/mod.rs | 7 ++++--- src/cargo/ops/resolve.rs | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 2b751d6a890..4b633c26d69 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -57,7 +57,7 @@ use url::Url; use core::{PackageId, Registry, SourceId, Summary, Dependency}; use core::PackageIdSpec; -use core::shell::Shell; +use util::config::Config; use util::Graph; use util::errors::{CargoResult, CargoError}; use util::profile; @@ -342,7 +342,7 @@ type Activations = HashMap>>; pub fn resolve(summaries: &[(Summary, Method)], replacements: &[(PackageIdSpec, Dependency)], registry: &mut Registry, - shell: Option<&mut Shell>) -> CargoResult { + config: Option<&Config>) -> CargoResult { let cx = Context { resolve_graph: RcList::new(), resolve_features: HashMap::new(), @@ -377,7 +377,8 @@ pub fn resolve(summaries: &[(Summary, Method)], trace!("resolved: {:?}", resolve); // If we have a shell, emit warnings about required deps used as feature. - if let Some(shell) = shell { + 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)?; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 074f798985a..cd88738cae4 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -257,17 +257,15 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, None => root_replace.to_vec(), }; - let mut shell; - let opt_shell = if warn { - shell = ws.config().shell(); - Some(&mut *shell) + let config = if warn { + Some(ws.config()) } else { None }; let mut resolved = resolver::resolve(&summaries, &replace, registry, - opt_shell)?; + config)?; resolved.register_used_patches(registry.patches()); if let Some(previous) = previous { resolved.merge_from(previous)?;