From: Dirkjan Ochtman Date: Thu, 5 Apr 2018 14:59:51 +0000 (+0200) Subject: Take feature namespace into account while building summary (fixes #1286) X-Git-Tag: archive/raspbian/0.35.0-2+rpi1~3^2^2^2^2^2^2^2~22^2~1^2~36^2~3 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=cb533ae1bf363606994be75eb533ccc7a542a81a;p=cargo.git Take feature namespace into account while building summary (fixes #1286) Here's an attempt at a table to cover the different cases: Feature Old (must be in features table) Continue Namespaced (might be stray value) In features table: Check that Crate dependency is in the list -> Non-optional dependency: Bail [PREVIOUSLY: bailed for non-optional dependency] -> Optional dependency: Insert feature of this name -> Else: Bail [PREVIOUSLY: bailed for unknown dependency or feature] Crate Old (might be stray value) Non-optional dependency: Bail No dependency found: Bail Namespaced Non-optional dependency: Bail No dependency found: Bail CrateFeature Old No dependency found: Bail Namespaced No dependency found: Bail --- diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index cfe1407ed..22c923ad9 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -38,7 +38,7 @@ impl Summary { namespaced_features: bool, ) -> CargoResult { for dep in dependencies.iter() { - if features.get(&*dep.name()).is_some() { + if !namespaced_features && features.get(&*dep.name()).is_some() { bail!( "Features and dependencies cannot have the \ same name: `{}`", @@ -52,7 +52,7 @@ impl Summary { ) } } - let feature_map = build_feature_map(features, &dependencies)?; + let feature_map = build_feature_map(features, &dependencies, namespaced_features)?; Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, @@ -137,6 +137,7 @@ impl PartialEq for Summary { fn build_feature_map( features: BTreeMap>, dependencies: &[Dependency], + namespaced: bool, ) -> CargoResult { use self::FeatureValue::*; let dep_map: HashMap<_, _> = dependencies @@ -146,52 +147,169 @@ fn build_feature_map( let mut map = BTreeMap::new(); for (feature, list) in features.iter() { + // If namespaced features is active and the key is the same as that of an + // optional dependency, that dependency must be included in the values. + // Thus, if a `feature` is found that has the same name as a dependency, we + // (a) bail out if the dependency is non-optional, and (b) we track if the + // feature requirements include the dependency `crate:feature` in the list. + // This is done with the `dependency_found` variable, which can only be + // false if features are namespaced and the current feature key is the same + // as the name of an optional dependency. If so, it gets set to true during + // iteration over the list if the dependency is found in the list. + let mut dependency_found = if namespaced { + match dep_map.get(feature.as_str()) { + Some(ref dep_data) => { + if !dep_data.is_optional() { + bail!( + "Feature `{}` includes the dependency of the same name, but this is \ + left implicit in the features included by this feature.\n\ + Additionally, the dependency must be marked as optional to be \ + included in the feature definition.\n\ + Consider adding `crate:{}` to this feature's requirements \ + and marking the dependency as `optional = true`", + feature, + feature + ) + } else { + false + } + } + None => true, + } + } else { + true + }; + let mut values = vec![]; for dep in list { - let val = FeatureValue::build(InternedString::new(dep), |fs| features.contains_key(fs)); - if let &Feature(_) = &val { - values.push(val); - continue; - } + let val = FeatureValue::build( + InternedString::new(dep), + |fs| features.contains_key(fs), + namespaced, + ); // Find data for the referenced dependency... let dep_data = { match val { - Feature(_) => None, - Crate(ref dep_name) | CrateFeature(ref dep_name, _) => { + Feature(ref dep_name) | Crate(ref dep_name) | CrateFeature(ref dep_name, _) => { dep_map.get(dep_name.as_str()) } } }; + let is_optional_dep = dep_data.map_or(false, |d| d.is_optional()); + if let FeatureValue::Crate(ref dep_name) = val { + // If we have a dependency value, check if this is the dependency named + // the same as the feature that we were looking for. + if !dependency_found && feature == dep_name.as_str() { + dependency_found = true; + } + } - match (&val, dep_data) { - (&Crate(ref dep), Some(d)) => { - if !d.is_optional() { - bail!( - "Feature `{}` depends on `{}` which is not an \ - optional dependency.\nConsider adding \ - `optional = true` to the dependency", - feature, - dep - ) + match (&val, dep_data.is_some(), is_optional_dep) { + // The value is a feature. If features are namespaced, this just means + // it's not prefixed with `crate:`, so we have to check whether the + // feature actually exist. If the feature is not defined *and* an optional + // dependency of the same name exists, the feature is defined implicitly + // here by adding it to the feature map, pointing to the dependency. + // If features are not namespaced, it's been validated as a feature already + // while instantiating the `FeatureValue` in `FeatureValue::build()`, so + // we don't have to do so here. + (&Feature(feat), _, true) => { + if namespaced && !features.contains_key(&*feat) { + map.insert(feat.to_string(), vec![FeatureValue::Crate(feat)]); } } - (&CrateFeature(ref dep_name, _), None) => bail!( + // If features are namespaced and the value is not defined as a feature + // and there is no optional dependency of the same name, error out. + // If features are not namespaced, there must be an existing feature + // here (checked by `FeatureValue::build()`), so it will always be defined. + (&Feature(feat), dep_exists, false) => { + if namespaced && !features.contains_key(&*feat) { + if dep_exists { + bail!( + "Feature `{}` includes `{}` which is not defined as a feature.\n\ + A non-optional dependency of the same name is defined; consider \ + adding `optional = true` to its definition", + feature, + feat + ) + } else { + bail!( + "Feature `{}` includes `{}` which is not defined as a feature", + feature, + feat + ) + } + } + } + // The value is a dependency. If features are namespaced, it is explicitly + // tagged as such (`crate:value`). If features are not namespaced, any value + // not recognized as a feature is pegged as a `Crate`. Here we handle the case + // where the dependency exists but is non-optional. It branches on namespaced + // just to provide the correct string for the crate dependency in the error. + (&Crate(ref dep), true, false) => if namespaced { + bail!( + "Feature `{}` includes `crate:{}` which is not an \ + optional dependency.\nConsider adding \ + `optional = true` to the dependency", + feature, + dep + ) + } else { + bail!( + "Feature `{}` depends on `{}` which is not an \ + optional dependency.\nConsider adding \ + `optional = true` to the dependency", + feature, + dep + ) + }, + // If namespaced, the value was tagged as a dependency; if not namespaced, + // this could be anything not defined as a feature. This handles the case + // where no such dependency is actually defined; again, the branch on + // namespaced here is just to provide the correct string in the error. + (&Crate(ref dep), false, _) => if namespaced { + bail!( + "Feature `{}` includes `crate:{}` which is not a known \ + dependency", + feature, + dep + ) + } else { + bail!( + "Feature `{}` includes `{}` which is neither a dependency nor \ + another feature", + feature, + dep + ) + }, + (&Crate(_), true, true) => {} + // If the value is a feature for one of the dependencies, bail out if no such + // dependency is actually defined in the manifest. + (&CrateFeature(ref dep, _), false, _) => bail!( "Feature `{}` requires a feature of `{}` which is not a \ dependency", feature, - dep_name - ), - (&Crate(ref dep), None) => bail!( - "Feature `{}` includes `{}` which is neither \ - a dependency nor another feature", - feature, dep ), - (&CrateFeature(_, _), Some(_)) | (&Feature(_), _) => {} + (&CrateFeature(_, _), true, _) => {} } values.push(val); } + + if !dependency_found { + // If we have not found the dependency of the same-named feature, we should + // bail here. + bail!( + "Feature `{}` includes the optional dependency of the \ + same name, but this is left implicit in the features \ + included by this feature.\nConsider adding \ + `crate:{}` to this feature's requirements.", + feature, + feature + ) + } + map.insert(feature.clone(), values); } Ok(map) @@ -212,30 +330,42 @@ pub enum FeatureValue { } impl FeatureValue { - fn build(feature: InternedString, is_feature: T) -> FeatureValue + fn build(feature: InternedString, is_feature: T, namespaced: bool) -> FeatureValue where T: Fn(&str) -> bool, { - match feature.find('/') { - Some(pos) => { + match (feature.find('/'), namespaced) { + (Some(pos), _) => { let (dep, dep_feat) = feature.split_at(pos); let dep_feat = &dep_feat[1..]; FeatureValue::CrateFeature(InternedString::new(dep), InternedString::new(dep_feat)) } - None if is_feature(&feature) => FeatureValue::Feature(feature), - None => FeatureValue::Crate(feature), + (None, true) if feature.starts_with("crate:") => { + FeatureValue::Crate(InternedString::new(&feature[6..])) + } + (None, true) => FeatureValue::Feature(feature), + (None, false) if is_feature(&feature) => FeatureValue::Feature(feature), + (None, false) => FeatureValue::Crate(feature), } } pub fn new(feature: InternedString, s: &Summary) -> FeatureValue { - Self::build(feature, |fs| s.features().contains_key(fs)) + Self::build( + feature, + |fs| s.features().contains_key(fs), + s.namespaced_features(), + ) } - pub fn to_string(&self) -> String { + pub fn to_string(&self, s: &Summary) -> String { use self::FeatureValue::*; match *self { Feature(ref f) => f.to_string(), - Crate(ref c) => c.to_string(), + Crate(ref c) => if s.namespaced_features() { + format!("crate:{}", &c) + } else { + c.to_string() + }, CrateFeature(ref c, ref f) => [c.as_ref(), f.as_ref()].join("/"), } } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 33683c6b1..2e1f67142 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -214,13 +214,14 @@ fn transmit( return Ok(()); } - let string_features = pkg.summary() + let summary = pkg.summary(); + let string_features = summary .features() .iter() .map(|(feat, values)| { ( feat.clone(), - values.iter().map(|fv| fv.to_string()).collect(), + values.iter().map(|fv| fv.to_string(&summary)).collect(), ) }) .collect::>>();