From e87f1516a02e49b768e1975cc2f11045e4554a5d Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 17:16:13 +0200 Subject: [PATCH 1/8] Move resolution requirement state into a separate struct --- src/cargo/core/resolver/mod.rs | 133 ++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 61 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index b605742d4ed..10092a563ee 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -859,55 +859,32 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool { a.patch == b.patch } -// 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 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, (bool, Vec)>, HashSet<&'a str>)> { - let mut deps = HashMap::new(); - let mut used = HashSet::new(); - let mut visited = HashSet::new(); - match *method { - Method::Everything => { - for key in s.features().keys() { - add_feature(s, key, &mut deps, &mut used, &mut visited)?; - } - for dep in s.dependencies().iter().filter(|d| d.is_optional()) { - add_feature(s, dep.name(), &mut deps, &mut used, - &mut visited)?; - } - } - Method::Required { features: requested_features, .. } => { - for feat in requested_features.iter() { - add_feature(s, feat, &mut deps, &mut used, &mut visited)?; - } - } - } - match *method { - Method::Everything | - Method::Required { uses_default_features: true, .. } => { - if s.features().get("default").is_some() { - add_feature(s, "default", &mut deps, &mut used, - &mut visited)?; - } +struct Requirements<'a> { + summary: &'a Summary, + // The deps 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 boolean indicates whether this + // package was specifically requested (rather than just requesting features + // *within* this package). + deps: HashMap<&'a str, (bool, Vec)>, + // The 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. + used: HashSet<&'a str>, + visited: HashSet<&'a str>, +} + +impl<'r> Requirements<'r> { + fn new<'a>(summary: &'a Summary) -> Requirements<'a> { + Requirements { + summary, + deps: HashMap::new(), + used: HashSet::new(), + visited: HashSet::new(), } - Method::Required { uses_default_features: false, .. } => {} } - return Ok((deps, used)); - fn add_feature<'a>(s: &'a Summary, - feat: &'a str, - deps: &mut HashMap<&'a str, (bool, Vec)>, - used: &mut HashSet<&'a str>, - visited: &mut HashSet<&'a str>) -> CargoResult<()> { + fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } // If this feature is of the form `foo/bar`, then we just lookup package @@ -920,8 +897,8 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) match parts.next() { Some(feat) => { let package = feat_or_package; - used.insert(package); - deps.entry(package) + self.used.insert(package); + self.deps.entry(package) .or_insert((false, Vec::new())) .1.push(feat.to_string()); } @@ -929,12 +906,12 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) let feat = feat_or_package; //if this feature has already been added, then just return Ok - if !visited.insert(feat) { + if !self.visited.insert(feat) { return Ok(()); } - used.insert(feat); - match s.features().get(feat) { + self.used.insert(feat); + match self.summary.features().get(feat) { Some(recursive) => { // This is a feature, add it recursively. for f in recursive { @@ -943,12 +920,12 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) on itself", feat); } - add_feature(s, f, deps, used, visited)?; + self.add_feature(f)?; } } None => { // This is a dependency, mark it as explicitly requested. - deps.entry(feat).or_insert((false, Vec::new())).0 = true; + self.deps.entry(feat).or_insert((false, Vec::new())).0 = true; } } } @@ -957,6 +934,39 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) } } +// Takes requested features for a single package from the input Method and +// recurses to find all requested features, dependencies and requested +// dependency features in a Requirements object, returning it to the resolver. +fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method) + -> CargoResult> { + let mut reqs = Requirements::new(s); + match *method { + Method::Everything => { + for key in s.features().keys() { + reqs.add_feature(key)?; + } + for dep in s.dependencies().iter().filter(|d| d.is_optional()) { + reqs.add_feature(dep.name())?; + } + } + Method::Required { features: requested_features, .. } => { + for feat in requested_features.iter() { + reqs.add_feature(feat)?; + } + } + } + match *method { + Method::Everything | + Method::Required { uses_default_features: true, .. } => { + if s.features().get("default").is_some() { + reqs.add_feature("default")?; + } + } + Method::Required { uses_default_features: false, .. } => {} + } + return Ok(reqs); +} + impl<'a> Context<'a> { // Activate this summary by inserting it into our list of known activations. // @@ -1117,18 +1127,18 @@ impl<'a> Context<'a> { let deps = s.dependencies(); let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps); - let (mut feature_deps, used_features) = build_features(s, method)?; + let mut reqs = build_requirements(s, method)?; let mut ret = Vec::new(); // Next, collect all actually enabled dependencies and their features. for dep in deps { // Skip optional dependencies, but not those enabled through a feature - if dep.is_optional() && !feature_deps.contains_key(dep.name()) { + if dep.is_optional() && !reqs.deps.contains_key(dep.name()) { continue } // 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![])); + let base = reqs.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 \ @@ -1151,21 +1161,22 @@ impl<'a> Context<'a> { // 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 !reqs.deps.is_empty() { + let unknown = reqs.deps.keys() + .map(|s| &s[..]) + .collect::>(); 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() { + if !reqs.used.is_empty() { let pkgid = s.package_id(); let set = self.resolve_features.entry(pkgid.clone()) .or_insert_with(HashSet::new); - for feature in used_features { + for feature in reqs.used { if !set.contains(feature) { set.insert(feature.to_string()); } From e1303ecc9623ae878c9305cc268efe16e0c66330 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 17:54:35 +0200 Subject: [PATCH 2/8] Extract method for requiring crate features --- src/cargo/core/resolver/mod.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 10092a563ee..699ea5c66ad 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -884,6 +884,13 @@ impl<'r> Requirements<'r> { } } + fn require_crate_feature(&mut self, package: &'r str, feat: &'r str) { + self.used.insert(package); + self.deps.entry(package) + .or_insert((false, Vec::new())) + .1.push(feat.to_string()); + } + fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } @@ -896,11 +903,7 @@ impl<'r> Requirements<'r> { let feat_or_package = parts.next().unwrap(); match parts.next() { Some(feat) => { - let package = feat_or_package; - self.used.insert(package); - self.deps.entry(package) - .or_insert((false, Vec::new())) - .1.push(feat.to_string()); + self.require_crate_feature(feat_or_package, feat); } None => { let feat = feat_or_package; From 63281c7acb04598be351aa141804af5c79ea89b0 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 17:57:24 +0200 Subject: [PATCH 3/8] Extract function to record if a feature was seen before --- src/cargo/core/resolver/mod.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 699ea5c66ad..8b512b90eef 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -891,6 +891,15 @@ impl<'r> Requirements<'r> { .1.push(feat.to_string()); } + fn seen(&mut self, feat: &'r str) -> bool { + if self.visited.insert(feat) { + self.used.insert(feat); + false + } else { + true + } + } + fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } @@ -907,13 +916,9 @@ impl<'r> Requirements<'r> { } None => { let feat = feat_or_package; - - //if this feature has already been added, then just return Ok - if !self.visited.insert(feat) { + if self.seen(feat) { return Ok(()); } - - self.used.insert(feat); match self.summary.features().get(feat) { Some(recursive) => { // This is a feature, add it recursively. From c3c387db73b73922d920352569bae25db248120d Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 17:59:41 +0200 Subject: [PATCH 4/8] Extract method for recording a requested dependency --- src/cargo/core/resolver/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8b512b90eef..4587fc08c98 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -900,6 +900,10 @@ impl<'r> Requirements<'r> { } } + fn require_dependency(&mut self, pkg: &'r str) { + self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true; + } + fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } @@ -933,7 +937,7 @@ impl<'r> Requirements<'r> { } None => { // This is a dependency, mark it as explicitly requested. - self.deps.entry(feat).or_insert((false, Vec::new())).0 = true; + self.require_dependency(feat); } } } From 1b01e36f14a814bfbbe6ef0583811079f473432f Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 18:04:28 +0200 Subject: [PATCH 5/8] Extract method for handling feature requirements --- src/cargo/core/resolver/mod.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4587fc08c98..fd16ccf13ec 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -904,6 +904,16 @@ impl<'r> Requirements<'r> { self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true; } + fn require_feature(&mut self, feat: &'r str, recursive: &'r Vec) -> CargoResult<()> { + for f in recursive { + if f == &feat { + bail!("Cyclic feature dependency: feature `{}` depends on itself", feat); + } + self.add_feature(f)?; + } + Ok(()) + } + fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> { if feat.is_empty() { return Ok(()) } @@ -925,15 +935,7 @@ impl<'r> Requirements<'r> { } match self.summary.features().get(feat) { Some(recursive) => { - // This is a feature, add it recursively. - for f in recursive { - if f == feat { - bail!("Cyclic feature dependency: feature `{}` depends \ - on itself", feat); - } - - self.add_feature(f)?; - } + self.require_feature(feat, recursive)?; } None => { // This is a dependency, mark it as explicitly requested. From 827887624b20adc2ecb21857000e2d0c204eb0e5 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 2 Oct 2017 18:06:32 +0200 Subject: [PATCH 6/8] Move shared logic into separated methods --- src/cargo/core/resolver/mod.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index fd16ccf13ec..45172a1dd15 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -901,10 +901,16 @@ impl<'r> Requirements<'r> { } fn require_dependency(&mut self, pkg: &'r str) { + if self.seen(pkg) { + return; + } self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true; } fn require_feature(&mut self, feat: &'r str, recursive: &'r Vec) -> CargoResult<()> { + if self.seen(feat) { + return Ok(()); + } for f in recursive { if f == &feat { bail!("Cyclic feature dependency: feature `{}` depends on itself", feat); @@ -929,17 +935,12 @@ impl<'r> Requirements<'r> { self.require_crate_feature(feat_or_package, feat); } None => { - let feat = feat_or_package; - if self.seen(feat) { - return Ok(()); - } - match self.summary.features().get(feat) { + match self.summary.features().get(feat_or_package) { Some(recursive) => { - self.require_feature(feat, recursive)?; + self.require_feature(feat_or_package, recursive)?; } None => { - // This is a dependency, mark it as explicitly requested. - self.require_dependency(feat); + self.require_dependency(feat_or_package); } } } From 8890568091f692061640687978a420793c7e49f4 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 4 Oct 2017 20:24:51 +0200 Subject: [PATCH 7/8] Move logic for walking over dependent features around --- src/cargo/core/resolver/mod.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 45172a1dd15..28740cb22f4 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -907,11 +907,11 @@ impl<'r> Requirements<'r> { self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true; } - fn require_feature(&mut self, feat: &'r str, recursive: &'r Vec) -> CargoResult<()> { + fn require_feature(&mut self, feat: &'r str) -> CargoResult<()> { if self.seen(feat) { return Ok(()); } - for f in recursive { + for f in self.summary.features().get(feat).expect("must be a valid feature") { if f == &feat { bail!("Cyclic feature dependency: feature `{}` depends on itself", feat); } @@ -935,13 +935,10 @@ impl<'r> Requirements<'r> { self.require_crate_feature(feat_or_package, feat); } None => { - match self.summary.features().get(feat_or_package) { - Some(recursive) => { - self.require_feature(feat_or_package, recursive)?; - } - None => { - self.require_dependency(feat_or_package); - } + if self.summary.features().contains_key(feat_or_package) { + self.require_feature(feat_or_package)?; + } else { + self.require_dependency(feat_or_package); } } } From abf4122f0f3bc33f3f0c9295ec994c459aa74dae Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 4 Oct 2017 20:26:30 +0200 Subject: [PATCH 8/8] Use specific methods to add requirements --- src/cargo/core/resolver/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 28740cb22f4..63f20a6cfba 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -955,10 +955,10 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method) match *method { Method::Everything => { for key in s.features().keys() { - reqs.add_feature(key)?; + reqs.require_feature(key)?; } for dep in s.dependencies().iter().filter(|d| d.is_optional()) { - reqs.add_feature(dep.name())?; + reqs.require_dependency(dep.name()); } } Method::Required { features: requested_features, .. } => { @@ -971,7 +971,7 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method) Method::Everything | Method::Required { uses_default_features: true, .. } => { if s.features().get("default").is_some() { - reqs.add_feature("default")?; + reqs.require_feature("default")?; } } Method::Required { uses_default_features: false, .. } => {}