From 116a3cd20db62aa7f397b305d87c7e6530fe0a8a Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Sat, 14 Oct 2017 11:18:50 +0200 Subject: [PATCH 1/3] Define FeatureMap type as an abstraction --- src/cargo/core/mod.rs | 2 +- src/cargo/core/package.rs | 6 +++--- src/cargo/core/summary.rs | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index f6434f77362..6b8926756e1 100644 --- a/src/cargo/core/mod.rs +++ b/src/cargo/core/mod.rs @@ -9,7 +9,7 @@ pub use self::registry::Registry; pub use self::resolver::Resolve; pub use self::shell::{Shell, Verbosity}; pub use self::source::{GitReference, Source, SourceId, SourceMap}; -pub use self::summary::Summary; +pub use self::summary::{FeatureMap, Summary}; pub use self::workspace::{Members, Workspace, WorkspaceConfig, WorkspaceRootConfig}; pub mod source; diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index f3ce2eaf29c..990144254da 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,5 +1,5 @@ use std::cell::{Ref, RefCell}; -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::fmt; use std::hash; use std::path::{Path, PathBuf}; @@ -10,7 +10,7 @@ use toml; use lazycell::LazyCell; use core::{Dependency, Manifest, PackageId, SourceId, Target}; -use core::{SourceMap, Summary}; +use core::{FeatureMap, SourceMap, Summary}; use core::interning::InternedString; use util::{internal, lev_distance, Config}; use util::errors::{CargoResult, CargoResultExt}; @@ -39,7 +39,7 @@ struct SerializedPackage<'a> { source: &'a SourceId, dependencies: &'a [Dependency], targets: &'a [Target], - features: &'a BTreeMap>, + features: &'a FeatureMap, manifest_path: &'a str, } diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index db2545d44c5..1081b5d6c4b 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -21,7 +21,7 @@ pub struct Summary { struct Inner { package_id: PackageId, dependencies: Vec, - features: BTreeMap>, + features: FeatureMap, checksum: Option, links: Option, } @@ -110,7 +110,7 @@ impl Summary { pub fn dependencies(&self) -> &[Dependency] { &self.inner.dependencies } - pub fn features(&self) -> &BTreeMap> { + pub fn features(&self) -> &FeatureMap { &self.inner.features } pub fn checksum(&self) -> Option<&str> { @@ -158,3 +158,5 @@ impl PartialEq for Summary { self.inner.package_id == other.inner.package_id } } + +pub type FeatureMap = BTreeMap>; From 7b542268f5a36edd104242fad365f61c2d6b9d47 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Sun, 15 Oct 2017 14:23:33 +0200 Subject: [PATCH 2/3] Introduce FeatureValue enum for tracking feature types --- src/cargo/core/interning.rs | 11 ++++ src/cargo/core/mod.rs | 2 +- src/cargo/core/resolver/context.rs | 74 +++++++++++----------- src/cargo/core/summary.rs | 99 +++++++++++++++++++++++------- src/cargo/ops/registry.rs | 14 ++++- tests/testsuite/metadata.rs | 4 +- 6 files changed, 143 insertions(+), 61 deletions(-) diff --git a/src/cargo/core/interning.rs b/src/cargo/core/interning.rs index 2f8cf7451fa..d8c18df2d84 100644 --- a/src/cargo/core/interning.rs +++ b/src/cargo/core/interning.rs @@ -1,3 +1,5 @@ +use serde::{Serialize, Serializer}; + use std::fmt; use std::sync::RwLock; use std::collections::HashSet; @@ -95,3 +97,12 @@ impl PartialOrd for InternedString { Some(self.cmp(other)) } } + +impl Serialize for InternedString { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(self.inner) + } +} diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index 6b8926756e1..2f7db061a21 100644 --- a/src/cargo/core/mod.rs +++ b/src/cargo/core/mod.rs @@ -9,7 +9,7 @@ pub use self::registry::Registry; pub use self::resolver::Resolve; pub use self::shell::{Shell, Verbosity}; pub use self::source::{GitReference, Source, SourceId, SourceMap}; -pub use self::summary::{FeatureMap, Summary}; +pub use self::summary::{FeatureMap, FeatureValue, Summary}; pub use self::workspace::{Members, Workspace, WorkspaceConfig, WorkspaceRootConfig}; pub mod source; diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index a1f97fe3c25..2de77d39fc3 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -1,7 +1,7 @@ use std::collections::{HashMap, HashSet}; use std::rc::Rc; -use core::{Dependency, PackageId, SourceId, Summary}; +use core::{Dependency, FeatureValue, PackageId, SourceId, Summary}; use core::interning::InternedString; use util::Graph; use util::CargoResult; @@ -170,7 +170,24 @@ impl Context { let deps = s.dependencies(); let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps); - let mut reqs = build_requirements(s, method)?; + // Requested features stored in the Method are stored as string references, but we want to + // transform them into FeatureValues here. In order to pass the borrow checker with + // storage of the FeatureValues that outlives the Requirements object, we do the + // transformation here, and pass the FeatureValues to build_requirements(). + let values = if let Method::Required { + all_features: false, + features: requested, + .. + } = *method + { + requested + .iter() + .map(|f| FeatureValue::new(f, s)) + .collect::>() + } else { + vec![] + }; + let mut reqs = build_requirements(s, method, &values)?; let mut ret = Vec::new(); // Next, collect all actually enabled dependencies and their features. @@ -269,8 +286,12 @@ impl Context { fn build_requirements<'a, 'b: 'a>( s: &'a Summary, method: &'b Method, + requested: &'a [FeatureValue], ) -> CargoResult> { let mut reqs = Requirements::new(s); + for fv in requested.iter() { + reqs.require_value(fv)?; + } match *method { Method::Everything | Method::Required { @@ -283,12 +304,7 @@ fn build_requirements<'a, 'b: 'a>( reqs.require_dependency(dep.name().as_str()); } } - Method::Required { - features: requested_features, - .. - } => for feat in requested_features.iter() { - reqs.add_feature(feat)?; - }, + _ => {} // Explicitly requested features are handled through `requested` } match *method { Method::Everything @@ -359,50 +375,34 @@ impl<'r> Requirements<'r> { } fn require_feature(&mut self, feat: &'r str) -> CargoResult<()> { - if self.seen(feat) { + if feat.is_empty() || self.seen(feat) { return Ok(()); } - for f in self.summary + for fv in self.summary .features() .get(feat) .expect("must be a valid feature") { - if f == feat { - bail!( + match *fv { + FeatureValue::Feature(ref dep_feat) if **dep_feat == *feat => bail!( "Cyclic feature dependency: feature `{}` depends on itself", feat - ); + ), + _ => {} } - self.add_feature(f)?; + self.require_value(&fv)?; } Ok(()) } - 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 - // `foo` and enable its feature `bar`. Otherwise this feature is of the - // form `foo` and we need to recurse to enable the feature `foo` for our - // own package, which may end up enabling more features or just enabling - // a dependency. - let mut parts = feat.splitn(2, '/'); - let feat_or_package = parts.next().unwrap(); - match parts.next() { - Some(feat) => { - self.require_crate_feature(feat_or_package, feat); - } - None => { - if self.summary.features().contains_key(feat_or_package) { - self.require_feature(feat_or_package)?; - } else { - self.require_dependency(feat_or_package); - } + fn require_value(&mut self, fv: &'r FeatureValue) -> CargoResult<()> { + match *fv { + FeatureValue::Feature(ref feat) => self.require_feature(feat), + FeatureValue::Crate(ref dep) => Ok(self.require_dependency(dep)), + FeatureValue::CrateFeature(ref dep, ref dep_feat) => { + Ok(self.require_crate_feature(dep, dep_feat)) } } - Ok(()) } } diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 1081b5d6c4b..34cdc72b5f7 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -48,47 +48,60 @@ impl Summary { ) } } + let mut features_new = BTreeMap::new(); for (feature, list) in features.iter() { - for dep in list.iter() { - let mut parts = dep.splitn(2, '/'); - let dep = parts.next().unwrap(); - let is_reexport = parts.next().is_some(); - if !is_reexport && features.get(dep).is_some() { + let mut values = vec![]; + for dep in list { + use self::FeatureValue::*; + let val = FeatureValue::build(dep, |fs| (&features).get(fs).is_some()); + if let &Feature(_) = &val { + // Return early to avoid doing unnecessary work + values.push(val); continue; } - match dependencies.iter().find(|d| &*d.name() == dep) { - Some(d) => { - if d.is_optional() || is_reexport { - continue; + // Find data for the referenced dependency... + let dep_data = { + let dep_name = match &val { + &Feature(_) => "", + &Crate(ref dep_name) | &CrateFeature(ref dep_name, _) => dep_name, + }; + dependencies.iter().find(|d| *d.name() == *dep_name) + }; + 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 + ) } - bail!( - "Feature `{}` depends on `{}` which is not an \ - optional dependency.\nConsider adding \ - `optional = true` to the dependency", - feature, - dep - ) } - None if is_reexport => bail!( + (&CrateFeature(ref dep_name, _), None) => bail!( "Feature `{}` requires a feature of `{}` which is not a \ dependency", feature, - dep + dep_name ), - None => bail!( + (&Crate(ref dep), None) => bail!( "Feature `{}` includes `{}` which is neither \ a dependency nor another feature", feature, dep ), + (&CrateFeature(_, _), Some(_)) | (&Feature(_), _) => {} } + values.push(val); } + features_new.insert(feature.clone(), values); } Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, dependencies, - features, + features: features_new, checksum: None, links: links.map(|l| InternedString::new(&l)), }), @@ -159,4 +172,48 @@ impl PartialEq for Summary { } } -pub type FeatureMap = BTreeMap>; +/// FeatureValue represents the types of dependencies a feature can have: +/// +/// * Another feature +/// * An optional dependency +/// * A feature in a depedency +/// +/// The selection between these 3 things happens as part of the construction of the FeatureValue. +#[derive(Clone, Debug, Serialize)] +pub enum FeatureValue { + Feature(InternedString), + Crate(InternedString), + CrateFeature(InternedString, InternedString), +} + +impl FeatureValue { + fn build(feature: &str, is_feature: T) -> FeatureValue + where + T: Fn(&str) -> bool, + { + match feature.find('/') { + 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(InternedString::new(feature)), + None => FeatureValue::Crate(InternedString::new(feature)), + } + } + + pub fn new(feature: &str, s: &Summary) -> FeatureValue { + Self::build(feature, |fs| s.features().get(fs).is_some()) + } + + pub fn to_string(&self) -> String { + use self::FeatureValue::*; + match *self { + Feature(ref f) => f.to_string(), + Crate(ref c) => c.to_string(), + CrateFeature(ref c, ref f) => [c.as_ref(), f.as_ref()].join("/"), + } + } +} + +pub type FeatureMap = BTreeMap>; diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index ed997ccb266..64f4a8cb57d 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -1,4 +1,5 @@ use std::{cmp, env}; +use std::collections::BTreeMap; use std::fs::{self, File}; use std::iter::repeat; use std::time::Duration; @@ -213,12 +214,23 @@ fn transmit( return Ok(()); } + let string_features = pkg.summary() + .features() + .iter() + .map(|(feat, values)| { + ( + feat.clone(), + values.iter().map(|fv| fv.to_string()).collect(), + ) + }) + .collect::>>(); + let publish = registry.publish( &NewCrate { name: pkg.name().to_string(), vers: pkg.version().to_string(), deps, - features: pkg.summary().features().clone(), + features: string_features, authors: authors.clone(), description: description.clone(), homepage: homepage.clone(), diff --git a/tests/testsuite/metadata.rs b/tests/testsuite/metadata.rs index 4a2154bd930..34720c4b222 100644 --- a/tests/testsuite/metadata.rs +++ b/tests/testsuite/metadata.rs @@ -192,7 +192,9 @@ optional_feat = [] ], "features": { "default": [ - "default_feat" + { + "Feature": "default_feat" + } ], "default_feat": [], "optional_feat": [] From 762110884113fb0d10d92cd55ccd8812d1167f55 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Sat, 31 Mar 2018 17:34:19 +0200 Subject: [PATCH 3/3] Extract function to build feature map for readability --- src/cargo/core/summary.rs | 111 +++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 50 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 34cdc72b5f7..1c8d95b36e4 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -48,60 +48,12 @@ impl Summary { ) } } - let mut features_new = BTreeMap::new(); - for (feature, list) in features.iter() { - let mut values = vec![]; - for dep in list { - use self::FeatureValue::*; - let val = FeatureValue::build(dep, |fs| (&features).get(fs).is_some()); - if let &Feature(_) = &val { - // Return early to avoid doing unnecessary work - values.push(val); - continue; - } - // Find data for the referenced dependency... - let dep_data = { - let dep_name = match &val { - &Feature(_) => "", - &Crate(ref dep_name) | &CrateFeature(ref dep_name, _) => dep_name, - }; - dependencies.iter().find(|d| *d.name() == *dep_name) - }; - 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 - ) - } - } - (&CrateFeature(ref dep_name, _), None) => 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(_), _) => {} - } - values.push(val); - } - features_new.insert(feature.clone(), values); - } + let feature_map = build_feature_map(features, &dependencies)?; Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, dependencies, - features: features_new, + features: feature_map, checksum: None, links: links.map(|l| InternedString::new(&l)), }), @@ -172,6 +124,65 @@ impl PartialEq for Summary { } } +// Checks features for errors, bailing out a CargoResult:Err if invalid, +// and creates FeatureValues for each feature. +fn build_feature_map( + features: BTreeMap>, + dependencies: &Vec, +) -> CargoResult { + use self::FeatureValue::*; + let mut map = BTreeMap::new(); + for (feature, list) in features.iter() { + let mut values = vec![]; + for dep in list { + let val = FeatureValue::build(dep, |fs| (&features).get(fs).is_some()); + if let &Feature(_) = &val { + values.push(val); + continue; + } + + // Find data for the referenced dependency... + let dep_data = { + let dep_name = match &val { + &Feature(_) => "", + &Crate(ref dep_name) | &CrateFeature(ref dep_name, _) => dep_name, + }; + dependencies.iter().find(|d| *d.name() == *dep_name) + }; + + 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 + ) + } + } + (&CrateFeature(ref dep_name, _), None) => 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(_), _) => {} + } + values.push(val); + } + map.insert(feature.clone(), values); + } + Ok(map) +} + /// FeatureValue represents the types of dependencies a feature can have: /// /// * Another feature